Skip to content
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.

Sign up for GitHub

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

Jump to bottom

perf: Optimise render counts #849

Merged
merged 14 commits into from
Jan 8, 2025
Merged

perf: Optimise render counts #849

merged 14 commits into from
Jan 8, 2025

Conversation

franky47
Copy link
Member

@franky47 franky47 commented Jan 6, 2025 •
edited
Loading

The optimal number of renders should be:

  • On mount: 1 (with the correct search params)
  • On update, with shallow: true (the default): 2 (one for the internal state, and one when the URL has updated)
  • On update, with shallow: false: 2 to 3, depending of whether we do optimistic URL updates and the time it takes for the server to respond and "update" the URL again

Progress

  • useQueryStates now renders only once on mount (was 2 renders previously)
  • useQueryStates now renders one less time (detecting query cache hits)
  • Fixed a bug in the patch-history caching logic (for React Router & Remix), saving one extra render on shallow: false.
Baseline 
app router - useQueryState - shallow: true, history: replace, startTransition: false should then render 2 times on updates: expected 5 to equal 3 
app router - useQueryState - shallow: true, history: push, startTransition: false should then render 2 times on updates: expected 5 to equal 3 
app router - useQueryStates - shallow: true, history: replace, startTransition: false should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: true, history: replace, startTransition: false should then render 2 times on updates: expected 6 to equal 3
app router - useQueryStates - shallow: true, history: push, startTransition: false should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: true, history: push, startTransition: false should then render 2 times on updates: expected 6 to equal 3
app router - useQueryStates - shallow: false, history: replace, startTransition: false should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: false, history: replace, startTransition: false should then render 3 times on updates: expected 5 to equal 4
app router - useQueryStates - shallow: false, history: replace, startTransition: false, delay: 50ms should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: false, history: replace, startTransition: false, delay: 50ms should then render 3 times on updates: expected 6 to equal 4
app router - useQueryStates - shallow: false, history: replace, startTransition: true should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: false, history: replace, startTransition: true should then render 3 times on updates: expected 5 to equal 4
app router - useQueryStates - shallow: false, history: replace, startTransition: true, delay: 50ms should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: false, history: replace, startTransition: true, delay: 50ms should then render 3 times on updates: expected 6 to equal 4
app router - useQueryStates - shallow: false, history: push, startTransition: false should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: false, history: push, startTransition: false should then render 3 times on updates: expected 5 to equal 4
app router - useQueryStates - shallow: false, history: push, startTransition: false, delay: 50ms should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: false, history: push, startTransition: false, delay: 50ms should then render 3 times on updates: expected 5 to equal 4
app router - useQueryStates - shallow: false, history: push, startTransition: true should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: false, history: push, startTransition: true should then render 3 times on updates: expected 5 to equal 4
app router - useQueryStates - shallow: false, history: push, startTransition: true, delay: 50ms should render once on mount: expected 2 to equal 1
app router - useQueryStates - shallow: false, history: push, startTransition: true, delay: 50ms should then render 3 times on updates: expected 6 to equal 4

Tasks

  • Investigate why wrapping URL updates with startTransition causes 4 renders in the pages router -> maybe the isLoading flag?
  • Add test to React Router based frameworks (there were differences whether the loader function was defined or not, that will require two different pages)
  • Add test to React SPA
  • Investigate flakiness on test run (looks to be caused by CPU load, getting a lot more flakes on older machines when running 5 Cypress instances in parallel)
Click to expand 
  Render count - app router - useQueryState - shallow: false, history: push, startTransition: true
render
    ✓ should render once on mount (84ms)
render
    (Attempt 1 of 2) should then render 3 times on updates
      cy:command ✔  visit	/app/render-count/useQueryState/false/push/true?delay=0
      cy:command ✔  contains	#hydration-marker, hydrated
      cy:command ✔  assert	expected **<div#hydration-marker>** to be **hidden**
      cy:command ✔  stub-1	log("render")
      cy:command ✔  get	button
      cy:command ✔  click	
      cy:command ✔  stub-1	log("render")
      cy:command ✔  stub-1	log("render")
      cy:command ✔  new url	http://localhost:3001/base/app/render-count/useQueryState/false/push/true?delay=0&test=pass
        cy:fetch ➟  GET http://localhost:3001/base/app/render-count/useQueryState/false/push/true?delay=0&test=pass&_rsc=vqr5f
                    Status: 200
      cy:command ✔  stub-1	log("render")
      cy:command ✔  stub-1	log("render")
      cy:command ✔  get	@consoleLog
      cy:command ✘  assert	expected **5** to equal **4**
                    Actual: 	5
                    Expected: 	4


render
    1) should then render 3 times on updates
      cy:command ✔  visit	/app/render-count/useQueryState/false/push/true?delay=0
      cy:command ✔  contains	#hydration-marker, hydrated
      cy:command ✔  assert	expected **<div#hydration-marker>** to be **hidden**
      cy:command ✔  stub-1	log("render")
      cy:command ✔  get	button
      cy:command ✔  click	
      cy:command ✔  stub-1	log("render")
      cy:command ✔  stub-1	log("render")
      cy:command ✔  new url	http://localhost:3001/base/app/render-count/useQueryState/false/push/true?delay=0&test=pass
        cy:fetch ➟  GET http://localhost:3001/base/app/render-count/useQueryState/false/push/true?delay=0&test=pass&_rsc=vqr5f
                    Status: 200
      cy:command ✔  get	@consoleLog
      cy:command ✘  assert	expected **3** to equal **4**
                    Actual: 	3
                    Expected: 	4
      cy:command ✔  stub-1	log("render")

Notes

  • Render counts are wildly different on React Router versions (6 vs 7 vs Remix), since everything else is the same, it feels like we're testing the frameworks.. At least it gives us a baseline to go down from.
  • Defining startTransition almost always causes one extra render. That could be the isLoading flag toggling. It does not cause one in RRv6 though.

@vercel Vercel
Copy link

vercel bot commented Jan 6, 2025 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
nuqs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 8, 2025 2:03pm

@franky47 franky47 changed the title test: Add failing test for render counts perf: Optimise render counts Jan 6, 2025
@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Jan 6, 2025 •
edited
Loading

Open in Stackblitz

npm i https://pkg.pr.new/nuqs@849

commit: 809aa8f

@franky47 franky47 marked this pull request as ready for review January 8, 2025 13:41
franky47 added 12 commits January 8, 2025 14:45
@franky47
test: Add failing test for render counts
b1c75dc
@franky47
perf: Reduce re-render counts for useQueryStates
a3d6bc8 
1. Trigger the state sync effect on the state keys only
(as the keyMap is not usually referentially stable)
2. Build the queryRef from the tracked keys, not all search params
(which may be empty), and fix undefined vs null access when parsing.
3. When parsing, check if there is at least one miss before updating
the state (which causes a re-render).
@franky47
perf: Wrap the whole update in a transition
17f4e18
@franky47
test: Also test for correctness
ef33f0c
@franky47
test: Add Next.js pages router
07682fc 
Somehow wrapping the URL update in a startTransition causes a lot more re-renders.
@franky47
test: Add render count test for Remix
32d6c5c
@franky47
test: Add render count test for React Router
efcc760
@franky47
test: Remove extra renders caused by useTransition's isLoading toggle
6198c68
@franky47
test: Address test flakiness
70aac06 
Render counts seem to be highly susceptible to variance when
the CPU is under load (eg: running `pnpm run test` on the whole
codebase on an older machine), so we're increasing the number
of retries for those tests (but only in CI, as we want them to fail
fast in local development).
@franky47
test: Add React render count test
eb5f0da
@franky47
fix: Search cache formatting in patch-history (missing ?)
fba8956
@franky47
chore: Fix env var access
97b127e
@franky47 franky47 force-pushed the chore/render-count branch from 5363f1c to 97b127e Compare January 8, 2025 13:46
@franky47 franky47 added this to the 🪵 Backlog milestone Jan 8, 2025
@franky47 franky47 added adapters/remix Uses the Remix adapter adapters/react-router Uses the React Router adapter feature/useQueryStates labels Jan 8, 2025
@franky47 franky47 merged commit fa63c13 into next Jan 8, 2025
27 checks passed
@franky47 franky47 deleted the chore/render-count branch January 8, 2025 14:07
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025

🎉 This PR is included in version 2.3.1-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

This was referenced Jan 8, 2025
Open
Open
This was linked to issues Jan 8, 2025
Set query state causes that component to rerender 4 times #755
Open
Nuqs rerender count in NextJS #822
Open
@franky47 franky47 mentioned this pull request Jan 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
adapters/react-router Uses the React Router adapter adapters/remix Uses the Remix adapter feature/useQueryStates released on @beta
Projects
None yet
Milestone
🚀 Shipping next
Development

Successfully merging this pull request may close these issues.

Nuqs rerender count in NextJS Set query state causes that component to rerender 4 times
1 participant
@franky47