Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New maintainer? #86

Open
jessetane opened this issue Nov 8, 2022 · 14 comments
Open

New maintainer? #86

jessetane opened this issue Nov 8, 2022 · 14 comments

Comments

@jessetane
Copy link
Owner

Hi all, I no longer use this library does anyone want to take it over?

@Sleepful
Copy link

Sleepful commented Dec 5, 2022

are you still using queues in JS in some other way?

@jessetane
Copy link
Owner Author

Promise.all and friends have been enough for my needs recently

@Sleepful
Copy link

Sleepful commented Dec 8, 2022

Cool! This is what... a 200 LoC library? and over 1M weekly downloads in NPM. Neat!
I will be testing it for my own purposes for a bit.

@Sleepful
Copy link

Sleepful commented Dec 8, 2022

Are any of the open PRs relevant? They look like expired milk.
Same question about open issues.

@Sleepful
Copy link

Sleepful commented Dec 8, 2022

Do you still want to be involved with the library (making decisions or giving advice) after take-over?

@luigimorel
Copy link

hey @jessetane,
i have been looking at the code and i'd like to take over

@MaksimLavrenyuk
Copy link
Collaborator

Hi, @jessetane.

I made a fork of your project in order to unbind from the events dependency (its presence in browser-targeted packages can be critical)

Maybe you'll look at PR to incorporate changes in your project?

In the future I am ready to be the main attendant of your package

Multiple changes to the project:

Detached the dependency of events from the project. I wrote my own eventEmitter. Now the project has no production dependencies.
Migrated tests from tape to jest. Tape has no support in modern IDEs, such as Webstorm, which complicates the debugging process
Rewrote all the code in typescript to improve type-safety. The original type definitions had problems.
Migrated from standard linter to eslint with preset standard-with-typescript rules. The migration was required because of lack of ts-standard support in webstorm
Added project build to 2 module target systems - CommonJS and ESM.

PR:
#90

@jessetane
Copy link
Owner Author

Sorry for the delayed response here, thanks @Sleepful and @morelmiles for indicating your interest. I haven't made up my mind about what I want to do here and realized that I still have some projects in production that depend on older versions of this library so I am a little nervous about potential "left-pad" type scenarios...

Thinking out loud here but as far as I can tell, this module is still potentially relevant because it does one thing that built-in Promise tooling doesn't: throttling the number of concurrent async operations active at a time. I'd love to see us focus in on the best solution for that particular problem. Please correct me if I'm missing a built-in solution or misunderstanding anything here.

Really appreciate you just jumping in and sending a PR @MaksimLavrenyuk, I'm just gonna start by posting my initial reactions so you can get an idea of what I am looking for:

  • ESM only. Trying to support multiple module systems is unnecessary. Major version bump and let's move on
  • Promises only. Not a huge Promise fan myself but let's face it they are standard now, so same as above
  • We should use EventTarget instead of rolling our own event system as it is built-in in node and the browser
  • For testing, I'd like to use tap-esm. It's dead simple, tiny, works in the browser etc. Support for a particular IDE isn't high on my list for what should be a publicly reusable module
  • No typescript. I know this is unpopular right now but it's still non-standard and requires heavy build tooling and so doesn't feel right to me for modules

Some other miscellaneous bigger picture feelings:

  • Less is more. We should try to get rid of stuff if we can. Linting for example, do we need that?
  • Modules should be written using the bleeding edge of whatever is standard and available (or at least shimmable) in a modern web browser and node
  • Modules need to work without build tooling in the browser and node out of the box

@MaksimLavrenyuk
Copy link
Collaborator

@jessetane
I apologize for the delay.

  • ESM only. I agree, for the major version it is justified. I also suggest using ES6 (native class, destructuring-assignment, Set).
  • Promises only. Please specify what you mean.
  • EventTarget. Ok, I'll try to understand this approach
  • Tap-esm. The main reason for transferring tests to jest is the inability to connect the debugger built into code editors to the process of running tests. If this is not present, it creates a huge testing headache. I think this is a necessary complication, please pay attention to it.
  • No typescript. Well, there is some truth in that.
  • linting is required in projects with a large team, this is not the case here. You can get rid of it. By and large, I left it because it already existed and I did not know how you would feel about removing it.
  • "Modules need to work without build tooling in the browser and node out of the box". With this in mind, there really is no need for assembly systems.

Will update soon #90 and I will notify you.

@jessetane
Copy link
Owner Author

  • Promises: I meant support for them should be well thought out, if I recall correctly existing support was kind of tacked on
  • EventTarget: you can inherit from it class Queue extends EventTarget
  • testing: Can you be more specific about the thing that jest does that you like? I have always used devtools in the browser (and the --inspect flag in node) to step through code in a debugger, but debugging is not directly related to how I usually write tests... maybe I'm missing something. npm install jest takes 30 seconds and installs 278 dependencies, and afaict it doesn't work in the browser without build tooling. Open to other approaches but testing needs to work in the browser with minimal fuss and preferably without blowing up node_modules. Check out the test-browser script from rpc-engine for an example of what I have been doing recently. Surely this can be improved but it works in all browsers with only two dependencies: a static file http server (unfortunately you can't serve ES modules directly from the filesystem) and a shim for import-maps in safari

@MaksimLavrenyuk
Copy link
Collaborator

The main advantage is the ability to debug without leaving the IDE and the box/if you have the editor extension. I attach screenshots of what I mean.

On the other hand, using jest equals running in a node.js environment. For projects like this, which do not use browser api (DOM api e.t.c.), this is not critical.

Honestly, testing inside a browser is new to me, I've always used mocha/jest (in a node.js runtime environment).

I don't want the discussion to descend into listing the merits of one tool or another. For me it's not critical which testing tool to use. You described how to debug tests, it suits me fine. I am ready to implement it on your stack.

image
image

@jessetane
Copy link
Owner Author

Thanks for the additional info and for giving my approach a shot. Feedback welcome. I usually code with vim and debug with whatever browser is around. For what it's worth though, I just downloaded Webstorm as I've never used it before and opened rpc-engine as a project. If I click on the file test/index.js and click the debug icon debugging seems to work fine for me out of the box:

Screen Shot 2023-01-09 at 2 57 34 PM

@MaksimLavrenyuk
Copy link
Collaborator

@jessetane
I hope you didn't think I'd disappear.
#88 (comment)

I want to let you know that I haven't disappeared anywhere and am refactoring with your suggestions in mind.

I apologize for the delay, the main work takes a lot of time. I will surely send you PR with the changes described above.

@MaksimLavrenyuk
Copy link
Collaborator

MaksimLavrenyuk commented Feb 5, 2023

@jessetane

As promised, I updated the PR with changes. I suggest that future discussion move to PR #90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants