fix(react): Change to useLayoutEffect in useSnapshot #891
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t inside useSnapshot instead of useEffect
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
dai-shi
reviewed
May 3, 2024
tests/basic.test.tsx
Outdated
| @@ -173,6 +173,118 @@ it('no extra re-renders (render func calls in non strict mode)', async () => { | |||
| expect(renderFn2).lastCalledWith(2) | |||
| }) | |||
|
|
|||
| it('no extra re-renders with nested useSnapshot (render func calls in non strict mode)', async () => { | |||
There was a problem hiding this comment.
Can you move two tests to a new tests/optimization.test.tsx file? (that's how we do in Jotai repo.)
Unless we find a real problem, we consider it "optimization".
(Of course, when we have a test that breaks logically, it's no longer optimization.)
|
That makes complete sense. The existing basic test I worked from verifies
the core orthogonality guarantee, these just guard against a performance
regression.
…On Fri, May 3, 2024 at 8:32 AM Daishi Kato ***@***.***> wrote:
***@***.**** commented on this pull request.
Thanks for your investigation and your work on this.
Just a minor change request.
------------------------------
In tests/basic.test.tsx
<#891 (comment)>:
> @@ -173,6 +173,118 @@ it('no extra re-renders (render func calls in non strict mode)', async () => {
expect(renderFn2).lastCalledWith(2)
})
+it('no extra re-renders with nested useSnapshot (render func calls in non strict mode)', async () => {
Can you move two tests to a new tests/optimization.test.tsx file? (that's
how we do in Jotai repo.)
Unless we find a real problem, we consider it "optimization".
(Of course, when we have a test that breaks logically, it's no longer
optimization.)
—
Reply to this email directly, view it on GitHub
<#891 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAFCRXYGTRBI373APEGRATZAN7VNAVCNFSM6AAAAABHFNV5ACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZYGA2DANRTGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
dai-shi
approved these changes
May 3, 2024
dai-shi
changed the title
|
🙌 |
dai-shi
added a commit
that referenced
this pull request
May 15, 2024
* prepare for the next major version * [v2] breaking: do not throw promises (#813) * [v2] breaking: do not throw promises * use use * fix CI hopefully * fix CI hopefully 2 * fix CI hopefully 3 * fix CI hopefully 4 * fix CI hopefully 5 * any type for simplicity * [v2] breaking: do not copy initial objects (#815) * [v2] breaking: do not copy initial objects * fix deepClone * refactor * ah we need it * deep clone * minor fix * breaking: require TS 4.5 at least (#817) * TS minimal requirement v4.5 * wip: test old ts * remove downlevel-dts * simplify test * simplify test 2 * simplify test 3 * wip: useMaybePromise * wip: useMaybePromise 2 * wip: useMaybePromise 3 * rename back * [v2] breaking: drop "module" condition (#818) * run prettier * [v2] breaking: require react 18 and drop use-sync-external-store (#819) * [v2] breaking: require react 18 and drop use-sync-external-store * drop tests pre react 18 * wip: cannot use react 17 for prd test * drop production test which is impossible * esm? * fix regex * fix sed * [v2] breaking: remove deprecated features (#820) * remove depreacated useProxy macro * revert plugin-transform * remove two more babel packages * revert babel core * remove proxyWithComputed * remove addComputed * remove devtools deprecated option * [v2] breaking: remove derive-valtio dependency (#821) * [v2] breaking: drop UMD/SystemJS builds and simplify babel config (#822) * [v2] breaking: drop UMD/SystemJS builds and simplify babel config * format * 2.0.0-alpha.0 * run prettier * 2.0.0-alpha.1 * simplify ts test script * update react canary version * remove depreacated proxyWithHistory * 2.0.0-alpha.2 * update react canary * [v2] export Snapshot type (#856) * 2.0.0-beta.0 * [v2] drop es5 (#865) * breaking: compatibility with memo (#866) * update yarn lock * 2.0.0-beta.1 * [v2] fix: make affected per proxy (#868) * 2.0.0-beta.2 * [v2] fix rollup config for cjs (#873) * 2.0.0-beta.3 * [v2] migration guide (#878) * fix workflow file * fix: explicit package.json type field (#882) * 2.0.0-beta.4 * fix(react): Change to useLayoutEffect in useSnapshot (#891) * Address spurious consistency check re-renders by using useLayoutEffect inside useSnapshot instead of useEffect * Move regression tests for useSnapshot perf improvement to optimization test file * Update tests/optimization.test.tsx --------- Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com> * 2.0.0-beta.5 * chore package.json --------- Co-authored-by: Christopher Swasey <christopher.swasey@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Address spurious consistency check re-renders by using useLayoutEffect inside useSnapshot instead of useEffect
Related Issues or Discussions
Fixes #890
Summary
Changing from
useEffecttouseLayoutEffectfixes a timing issue inuseSnapshotwhere a React consistency check runs before the effect to store the cached snapshot, causing a spurious re-render.Check List
yarn run prettierfor formatting code and docs