Reduce rerendering by using useLayoutEffect #54
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
There was a problem hiding this comment.
Overall I don't have many gripes with using useLayoutEffect, except that we better have a good reason since React encourages it to be used sparingly. Mostly, I'm requesting changes to update the comment on useIsomorphicLayoutEffect, since it doesn't seem to match the test or the PR title.
src/__tests__/index-test.tsx
Outdated
|
|
||
| updateStoreWithoutAct({ bar: 11, foo: '11' }) | ||
| updateStoreWithoutAct({ bar: 12, foo: '12' }) | ||
| act(()=>{}) |
There was a problem hiding this comment.
just flushing all hooks before checking for rerenders counts. Not sure if it's needed
src/create.ts
Outdated
| // To get around it, we can conditionally useEffect on the server (no-op) and | ||
| // useLayoutEffect in the browser. We need useLayoutEffect to ensure the store | ||
| // subscription callback always has the selector from the latest render commit | ||
| // available, otherwise a store update may happen between render and the effect, |
There was a problem hiding this comment.
a store update may happen between render and the effect, which may cause missed updates
Are you sure this is true? This is precisely why we check for updates after subscribing on line 129 below.
otherwise a store update may occur before the subscription is created and an inconsistent state may be observed
Again, won't the updates check handle this?
There was a problem hiding this comment.
yea, useLayoutEffect not about subscriptions
| @@ -265,6 +270,27 @@ describe('redux-react-hook', () => { | |||
| expect(renderCount).toBe(2); | |||
| }); | |||
|
|
|||
| it('renders once if have multiple useMappedState', () => { | |||
There was a problem hiding this comment.
this seems to be the real reason to use useLayoutEffect, so why isn't this in the comment?
fixes #53