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

4.0 changes #67

Open
brainkim opened this issue Sep 3, 2020 · 4 comments
Open

4.0 changes #67

brainkim opened this issue Sep 3, 2020 · 4 comments

Comments

@brainkim
Copy link
Member

brainkim commented Sep 3, 2020

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 (Not that great for implementing transducers? #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. (Should the Repeater automatically stop when its executor fulfills? #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.

@brainkim brainkim pinned this issue Sep 3, 2020
yaacovCR added a commit to ardatan/graphql-tools that referenced this issue Mar 31, 2021
we don't need a pubsub to subscribe only to the first published valuee, we can use  an expectant store that returns a promise when the store does not yet contain the value, where all the relevant promises resolve on a future set.

this change means we can retire the in-house pubsub.

repeaters probably make in-house pubsub unnecessary anyway, and were not yet used elsewhere in the codebase

also somewhat relevant, see repeaterjs/repeater#67 (comment)
yaacovCR added a commit to ardatan/graphql-tools that referenced this issue Apr 12, 2021
we don't need a pubsub to subscribe only to the first published valuee, we can use  an expectant store that returns a promise when the store does not yet contain the value, where all the relevant promises resolve on a future set.

this change means we can retire the in-house pubsub.

repeaters probably make in-house pubsub unnecessary anyway, and were not yet used elsewhere in the codebase

also somewhat relevant, see repeaterjs/repeater#67 (comment)
yaacovCR added a commit to ardatan/graphql-tools that referenced this issue Apr 15, 2021
we don't need a pubsub to subscribe only to the first published valuee, we can use  an expectant store that returns a promise when the store does not yet contain the value, where all the relevant promises resolve on a future set.

this change means we can retire the in-house pubsub.

repeaters probably make in-house pubsub unnecessary anyway, and were not yet used elsewhere in the codebase

also somewhat relevant, see repeaterjs/repeater#67 (comment)
@n1ru4l
Copy link

n1ru4l commented Dec 26, 2021

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

@brainkim
Copy link
Member Author

brainkim commented Jan 3, 2022

@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
Copy link

n1ru4l commented Jan 3, 2022

@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
Copy link

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!

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

3 participants