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

Overloading connect with factory #279

Merged
merged 2 commits into from
Feb 4, 2016

Conversation

tgriesser
Copy link
Contributor

I had actually considered this before doing the connectFactory approach in #185. The issue was that it felt wrong to overload the arguments and require first calling the function to determine its return type.

That said, this is the cleanest implementation I could come up with. It checks if the argument in question has fn.length of 0 and also returns a function.

This breaks a few tests, but nothing critical - only on some spies or guarantees an arity-0 function is called a certain amount of times. I can clean those up if this looks alright otherwise.

@gaearon
Copy link
Contributor

gaearon commented Feb 3, 2016

Rather than look at arg count, can we determine whether they returned functions instead of objects lazily first time we call them from the constructor?

@tgriesser
Copy link
Contributor Author

Took another pass at it, it's a little more verbose / complicated, but does things lazily as you mentioned, all tests are passing and it avoids calling the mapping functions any more times than necessary.

If this looks alright I can add some tests.

@@ -14,65 +14,40 @@ const defaultMergeProps = (stateProps, dispatchProps, parentProps) => ({
...dispatchProps
})

function isFunction(fn) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave those checks inline.

@gaearon
Copy link
Contributor

gaearon commented Feb 3, 2016

Yes, this looks good. Please do add tests.

@tgriesser
Copy link
Contributor Author

Updated, rebased, tests added. Also noticed when writing the tests that a final shallow equality check on the final merged props would save a render if there is a custom mergeProps supplied and nothing has changed, so I added that as well.

expect(updatedCount).toBe(0)
expect(memoizedReturnCount).toBe(2)
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: mind killing the newline here?

@gaearon
Copy link
Contributor

gaearon commented Feb 4, 2016

That’s amazing, thank you so much for your work on this. A few more nits and it’s good to go.

@tgriesser
Copy link
Contributor Author

All set with a test and a bit of formatting polish

gaearon added a commit that referenced this pull request Feb 4, 2016
@gaearon gaearon merged commit adc805a into reduxjs:master Feb 4, 2016
@gaearon
Copy link
Contributor

gaearon commented Feb 4, 2016

Would you be up to contributing a section to Computing Derived Data recipe documenting that? I think a lot of people won’t learn about this useful feature otherwise.

@tgriesser
Copy link
Contributor Author

Sure thing, I'll take a look at doing this a little later.

@ellbee
Copy link
Contributor

ellbee commented Feb 9, 2016

@tgriesser I have started to update the Reselect docs to reflect this change. If you haven't started on this already, would you like me to carry on and document it?

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

Successfully merging this pull request may close these issues.

4 participants