Update mergeProps to optionally accept a factory style function #523
Conversation
Obviously that didn't happen...
src/connect/mergeProps.js
Outdated
|
|
||
| if (typeof nextMergedProps === 'function') { | ||
| nextMergedProps = nextMergedProps(stateProps, dispatchProps, ownProps) | ||
| } |
There was a problem hiding this comment.
I don't think this is complete; it looks like it's still going to call the original factory method every time through the loop. I'd expect to see mergeProps copied into a variable that is overwritten with its result when the result is a function, similar to wrapMapToPropsFunc
test/components/connect.spec.js
Outdated
| @connect( | ||
| state => state, | ||
| () => ({ sum }), | ||
| () => (stateProps, dispatchProps, parentProps) => ({ |
There was a problem hiding this comment.
The test should probably cover that the factory part of the mergeProps method is only called once, related to my previous comment. I'd do something like
() => {
mergePropFactoryCalls += 1;
return (stateProps, dispatchProps, parentProps) => ({
sum: () => dispatchProps.sum(stateProps.a, parentProps.b)
});
}
and then assert that it equals 1 after dispatching several actions. Or use the spy stuff.
There was a problem hiding this comment.
Makes sense. Should this be in a separate test or I'll augment the existing one?
|
Any update on this, @neeharv? |
…select'. (6 failing tests to go)
…ops pass. BREAKING CHANGE: requires setting an option parameter mapStateIsFactory/mapDispatchIsFactory to signal factory methods.
…hind-the-scenes behavior of new implementation.
* Remove Style section Obviously that didn't happen... * Add es2015 modules export from redux project
* updates docs for changes in v5 (WORK IN PROGRESS) * update docs for changes in v5 * removes unused code from example * adds a tags to api.md for direct linking
…duxjs#477) * groups factories options and move *areEqual defaults into connect * refactors connect so that new "factories" extension points aren't in main API
The `current` variable would be set to `null` when `clear()` was called, which would cause an exception if there was more than one listener. The new test would fail before this change.
|
@timdorr this slipped my mind completely! I'll work on the changes requested over this weekend |
|
Can you revise the branch on the latest master? There are a lot of junk commits in here. |
I have a very similar use case to #488
I've reused @bsideup's code as-is, with the same test. We've started moving to v5 in production, hoping to see this soon in there!