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

4 participants
@tgriesser
Collaborator

tgriesser commented Feb 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 3, 2016

Collaborator

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?

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser Feb 3, 2016

Collaborator

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.

Collaborator

tgriesser commented Feb 3, 2016

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.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 3, 2016

Collaborator

Yes, this looks good. Please do add tests.

Collaborator

gaearon commented Feb 3, 2016

Yes, this looks good. Please do add tests.

@tgriesser

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser Feb 4, 2016

Collaborator

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.

Collaborator

tgriesser commented Feb 4, 2016

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)
})

This comment has been minimized.

@gaearon

gaearon Feb 4, 2016

Collaborator

Nit: mind killing the newline here?

@gaearon

gaearon Feb 4, 2016

Collaborator

Nit: mind killing the newline here?

const finalMergeProps = mergeProps || defaultMergeProps
const doStatePropsDependOnOwnProps = finalMapStateToProps.length !== 1
const doDispatchPropsDependOnOwnProps = finalMapDispatchToProps.length !== 1
const checkMergedEquals = finalMergeProps !== defaultMergeProps

This comment has been minimized.

@gaearon

gaearon Feb 4, 2016

Collaborator

Can we add a new test for this optimization so we don't regress?

@gaearon

gaearon Feb 4, 2016

Collaborator

Can we add a new test for this optimization so we don't regress?

computeDispatchProps(store, props) {
if (!this.finalMapDispatchToProps) {
return this.configureFinalMapDispatch(store, props)
}

This comment has been minimized.

@gaearon

gaearon Feb 4, 2016

Collaborator

Nit: please add a newline here

@gaearon

gaearon Feb 4, 2016

Collaborator

Nit: please add a newline here

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 4, 2016

Collaborator

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

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser Feb 4, 2016

Collaborator

All set with a test and a bit of formatting polish

Collaborator

tgriesser commented Feb 4, 2016

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 4, 2016

Collaborator

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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser Feb 4, 2016

Collaborator

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

Collaborator

tgriesser commented Feb 4, 2016

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

@ellbee

This comment has been minimized.

Show comment
Hide comment
@ellbee

ellbee Feb 9, 2016

Member

@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?

Member

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?

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist May 16, 2016

Contributor

@tgriesser is this checkMergedEquals needed for some case or is it just an optimization?

Contributor

Andarist commented on src/components/connect.js in 4bb0087 May 16, 2016

@tgriesser is this checkMergedEquals needed for some case or is it just an optimization?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment