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.

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

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

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.

Copy link

vercel bot commented Jan 6, 2025

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
Copy link

pkg-pr-new bot commented Jan 6, 2025

Open in Stackblitz

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

commit: 809aa8f

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).
Somehow wrapping the URL update in a startTransition causes a lot more re-renders.
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 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
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 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
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