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

Enable factories/memoizers for mergeProps #516

Closed
johnzielke opened this issue Oct 9, 2016 · 3 comments
Closed

Enable factories/memoizers for mergeProps #516

johnzielke opened this issue Oct 9, 2016 · 3 comments
Milestone

Comments

@johnzielke
Copy link

johnzielke commented Oct 9, 2016

In this issue one supposed strategy to bind action creators to selectedState is to use mergeProps, which seems a good idea as opposed to passing state to mapDispatch for the reasons mentioned in the issue. But when using mergeProps to achieve this, i defeat any memoizers that try to return the same objects, because as mergeProps will always be called, it will always return new bound functions. As far as I can tell, mergeProps does not have the possibility of a factory method as mapState and mapDispatch have.

I currently try to workaround this problem by adding the memoizer in my dumb component, updating in the constructor and in componentWillUpdate()

Are there any reasons against this addition?
Or is it such an edge case that we do not even want the few lines of code, we need to add for that, and the little time this would additionally need on the creation of the component.

If this is the case, I feel like we should add a proposal on how to work around this somewhere (or just generally have a few about memoization and binding action creators).
And would the current workaround I use be valid here, or am I missing something there?

@jimbolla
Copy link
Contributor

PR #488 was to address this but it was written against the wrong branch. I think it would be a good feature to have, if someone wants to reimplement it against the next branch. The test from #488 is probably still usable though.

@jimbolla
Copy link
Contributor

#523 is also open to address this.

@timdorr timdorr added this to the 5.1 milestone Nov 8, 2016
@frankwallis
Copy link

frankwallis commented Nov 21, 2016

FWIW I created a library for doing memoized deep merging, but never got around to using it (I used a custom connect function instead). It anyone would like to use it, it can be found here: https://github.com/frankwallis/remerger.

@timdorr timdorr closed this as completed Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants