pmndrs / valtio

💊 Valtio makes proxy-state simple for React and Vanilla
http://valtio.pmnd.rs
MIT License
8.67k stars 241 forks source link

fix(react): Change to useLayoutEffect in useSnapshot #891

Closed endash closed 2 months ago

endash commented 2 months ago

Address spurious consistency check re-renders by using useLayoutEffect inside useSnapshot instead of useEffect

Related Issues or Discussions

Fixes #890

Summary

Changing from useEffect to useLayoutEffect fixes a timing issue in useSnapshot where a React consistency check runs before the effect to store the cached snapshot, causing a spurious re-render.

Check List

vercel[bot] commented 2 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
valtio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 3, 2024 11:45pm
codesandbox-ci[bot] commented 2 months ago

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.

endash commented 2 months ago

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 https://github.com/pmndrs/valtio/pull/891#discussion_r1589138213:

@@ -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 https://github.com/pmndrs/valtio/pull/891#pullrequestreview-2038040632, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAFCRXYGTRBI373APEGRATZAN7VNAVCNFSM6AAAAABHFNV5ACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZYGA2DANRTGI . You are receiving this because you authored the thread.Message ID: @.***>

endash commented 2 months ago

🙌