Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.3k
Overloading connect with factory#279
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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? |
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. |
src/components/connect.js Outdated
There was a problem hiding this comment.
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.
Yes, this looks good. Please do add tests. |
55995db to43344c9CompareUpdated, 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 |
06de7fd to4bb0087Compare| expect(updatedCount).toBe(0) | ||
| expect(memoizedReturnCount).toBe(2) | ||
| }) | ||
There was a problem hiding this comment.
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?
That’s amazing, thank you so much for your work on this. A few more nits and it’s good to go. |
All set with a test and a bit of formatting polish |
Would you be up to contributing a section toComputing Derived Data recipe documenting that? I think a lot of people won’t learn about this useful feature otherwise. |
Sure thing, I'll take a look at doing this a little later. |
@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? |
I had actually considered this before doing the
connectFactoryapproach 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.lengthof 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.