repeaterjs / repeater

The missing constructor for creating safe async iterators
https://repeater.js.org
MIT License
459 stars 12 forks source link

4.0 changes #67

Open brainkim opened 4 years ago

brainkim commented 4 years ago

3.0 was released last November and I’m itching to do some breaking changes. Here’s what I’m planning:

  1. Move the combinator functions to their own module. EDIT: I’m starting to think the static methods should stay on the class, even if we implement the other combinator functions. I initially wrote combinators as static methods (Repeater.race) on the exported Repeater class, both on the assumption that they would be commonly used and because it draws a nice parallel with Promise.race. If repeaters ever end up becoming a standard thing, this might be how the functions are defined, but as a library, having the combinator functions exposed as static methods messes with the bundle size, and is not tree-shakeable. Additionally, it’s been brought to my attention (https://github.com/repeaterjs/repeater/issues/48) that unlike my previous assumption, there actually may be a need for repeaters in the implementation of combinators like map and filter and creating a module for these functions all defined in one place would be nice. I’ve checked out some other combinator libraries and it seems like many of them leak memory and cause deadlocks, so there is going to be value in creating a combinator module, especially for combinators like switchMap, which require a high degree of concurrency.

  2. Kill the monorepo. Package monorepos are annoying to deal with. I have made the mistake of not updating the internal versions of packages multiple times, there’s duplicated tooling across the packages, and I hate having to navigate a multi-level directory structure for what are essentially single-file modules. My plan is to get all the modules exported under the main repeater package and deprecate the other packages.

  3. Kill pubsub. I’m probably the only one using the pubsub module right now, but after a year of async iterator usage, what I believe is that you should always start with an EventTarget or callback-based abstraction when writing custom classes. Async iterators are high-level, always async abstractions and we shouldn’t encourage people to write abstractions which exclusively use them.

  4. Rename some functions. One thing I’m noticing is that the function names in limiters and timers are squatting on the expected name for the actual repeater object (const ??? = timeout(1000)). Most of the time you inline the function call directly in the for await statement, but for cases where you need to assign the repeater to a variable the naming can be kind of annoying. I’d like to maybe adopt some kind of consistent naming pattern like createTimeout, genTimeout or maybe sourceTimeout.

  5. Stop repeaters when the executor settles. EDIT: April 2021 I think this one also might not be worth it. I’m reading through code which uses Repeaters in the wild, and it might be a pain in the butt to have to fix all that code. This change strikes me as being less explicit and more confusing. (#49) Related to implementing combinators, I want to implicitly stop repeaters when the executor function returns. Whenever I write combinators I consistently feel compelled to write stop calls directly before the return statement or in a finally block. This is probably one of the more disruptive breaking changes as it will cause repeaters to stop unexpectedly if you didn’t await anything in the executor, but you can reconstruct the old behavior by awaiting or returning the stop promise. If your repeater executor exits synchronously, I encourage you to somehow await or return the stop promise to prepare for this change.

n1ru4l commented 2 years ago

I would really prefer removing the static methods from the class as this can make quite a difference for browser environments.

brainkim commented 2 years ago

@n1ru4l What are the reasons we can’t use static methods? They were added in “ES6,” before async iteration even, so I’m not sure what sort of environment removing static methods would benefit.

I initially defined the static methods as a way to mirror the Promise constructor and how it defines some key combinators (Promise.race(), Promise.all()). I’m still going back and forth on removing them, the key advantage being loose functions can be tree shaken, though these days I do not think the weight is big a deal.

n1ru4l commented 2 years ago

@brainkim We can use them, but having them as separate functions is definitely better for tree shaking. E.g. I have client-side code that does not use those static methods at all.

airhorns commented 11 months ago

Anyone made any progress on this? Noticing that this module is a really big contributor to the size of our browser bundles such that improving the tree-shake-ability would be huge!