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

Delay slice()ing the listeneres #8

Closed
gaearon opened this issue Apr 2, 2016 · 7 comments
Closed

Delay slice()ing the listeneres #8

gaearon opened this issue Apr 2, 2016 · 7 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Apr 2, 2016

Redux introduced a few optimizations in 3.x, and we now delay slice()ing listeners unless absolutely necessary, and don’t slice() them more than once per dispatch(). This project should probably adopt similar optimizations.

@tappleby
Copy link
Owner

tappleby commented Apr 2, 2016

Thanks for the heads up.

Was just looking over the redux release log an these look like the major changes:

reduxjs/redux@b7031ce
reduxjs/redux#1325
reduxjs/redux@c031c0a
reduxjs/redux@5b58608

I wonder if I can avoid duplicating this logic by wrapping the listener callback instead of reimplementing the subscribe logic.

@gaearon
Copy link
Contributor Author

gaearon commented Apr 2, 2016

Give it a try, let me know if there’s something I can do on our side

@tappleby
Copy link
Owner

tappleby commented Apr 3, 2016

So I ended up implementing both. Not sure if wrapping the listener ends up simplifying things, still end up needing all the tests as a regression.

Direct port of redux core: 4093d5c
Experimental version: ad8935d

One idea on the redux side that could help is moving the subscription logic into a util function this library could consume: tappleby/redux@6ca7c8d

@gaearon
Copy link
Contributor Author

gaearon commented Apr 13, 2016

Mind publishing this emitter on npm?

@tappleby
Copy link
Owner

I can, though does it make sense as a standalone package?

Also there's a bunch of emitters already on npm, wonder if it should be scoped to redux. eg redux-emitter or redux-core-emitter.

@tappleby
Copy link
Owner

So I merged the direct port (4093d5c) and released with v0.1.5. Wasnt sure the timeline for reduxjs/redux#1702, plus these optimizations will still be useful for non redux v4 users.

@gaearon
Copy link
Contributor Author

gaearon commented May 15, 2016

👍

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

2 participants