feat: Key isolation #855
Open
feat: Key isolation #855
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
commit: |
franky47
commented
Jan 9, 2025
| cy.get('#state').should('have.text', 'pass') | ||
| cy.location('search').should('contain', 'test=pass') | ||
| assertLogCount('render', expected.mount + expected.update) |
There was a problem hiding this comment.
note: Unrelated change to help the render count test be less flaky: by asserting the log count after testing for correctness, Cypress will wait for the URL to be correct (and give logs time to flush) before testing.
franky47
commented
Jan 9, 2025
| @@ -12,30 +17,38 @@ function updateUrl(search: URLSearchParams, options: AdapterOptions) { | |||
| url.search = renderQueryString(search) | |||
| const method = | |||
| options.history === 'push' ? history.pushState : history.replaceState | |||
| method.call(history, history.state, '', url) | |||
| method.call(history, history.state, historyUpdateMarker, url) | |||
There was a problem hiding this comment.
note: Looks like it this should be a bug fix for external history updates & sync optimisations, but it doesn't seem to make a difference. Better be aligned with the patch-history expected behaviour anyway.
| @@ -83,7 +83,7 @@ export function useQueryStates<KeyMap extends UseQueryStatesKeysMap>( | |||
| ), | |||
| [stateKeys, urlKeys] | |||
There was a problem hiding this comment.
note: urlKeys isn't guaranteed to be stable (if defined inline, it's not), so this would cause a cascade of referential instability.
Add a test & fix:
Suggested change
| [stateKeys, urlKeys] | |
| [stateKeys, JSON.stringify(urlKeys)] |
franky47
changed the title
franky47
added
adapters/remix
There was a problem hiding this comment.
Copilot reviewed 11 out of 26 changed files in this pull request and generated no comments.
Files not reviewed (15)
- packages/e2e/react/src/routes/key-isolation.useQueryState.tsx: Evaluated as low risk
- packages/e2e/react-router/v7/cypress/e2e/shared/key-isolation.cy.ts: Evaluated as low risk
- packages/e2e/react-router/v6/src/routes/key-isolation.useQueryState.tsx: Evaluated as low risk
- packages/e2e/react-router/v6/src/routes/key-isolation.useQueryStates.tsx: Evaluated as low risk
- packages/e2e/react/src/routes/key-isolation.useQueryStates.tsx: Evaluated as low risk
- packages/e2e/react-router/v7/app/routes/key-isolation.useQueryState.tsx: Evaluated as low risk
- packages/e2e/react-router/v7/app/routes/key-isolation.useQueryStates.tsx: Evaluated as low risk
- packages/e2e/react-router/v6/cypress/e2e/shared/key-isolation.cy.ts: Evaluated as low risk
- packages/e2e/react/cypress/e2e/shared/key-isolation.cy.ts: Evaluated as low risk
- packages/e2e/react/src/routes.tsx: Evaluated as low risk
- packages/e2e/react-router/v7/app/routes.ts: Evaluated as low risk
- packages/e2e/react-router/v6/src/react-router.tsx: Evaluated as low risk
- packages/nuqs/src/adapters/lib/context.ts: Evaluated as low risk
- packages/e2e/shared/specs/render-count.cy.ts: Evaluated as low risk
- packages/e2e/remix/app/routes/key-isolation.useQueryStates.tsx: Evaluated as low risk
Key isolation is the fact that updating a search param only re-renders hooks subscribed to this key. Most frameworks (Next.js & React Router) re-render every call site of their useSearchParams hooks when any search params change, causing unnecessary re-renders. Since in the React SPA we control the whole thing (there is no router so to speak), we can implement caching to detect only relevant differences and avoid re-rendering unnecessarily.
franky47
force-pushed
the
feat/url-key-subscription
branch
from
600ce91 to
3202da1
Compare
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.
Key isolation is the fact that updating a search param only re-renders hooks subscribed to this key.
Example:
<ComponentA>has auseQueryState('a')<ComponentB>has auseQueryState('b')Updating the state for
adoesn't re-renderComponentB.Most frameworks (Next.js & React Router) re-render every call site of their useSearchParams hooks when any search params change (via navigation events), causing unnecessary re-renders of
useQueryState(s). Some other routers like TanStack Router do implement key isolation.Since in the React SPA adapter we control the whole thing (there is no router so to speak), we can implement caching to detect only relevant differences and avoid re-rendering unnecessarily.
For React Router & Remix, we can do this too in shallow mode, since their
useSearchParamsdoesn't re-render on shallow updates. When usingshallow: false, everything would re-render after the URL has updated (when coming back from the loader). This feels like a natural behaviour that is in line with the frameworks navigation.For Next.js, this might require closer collaboration with the core team to implement this filtering built-in, because their
useSearchParamsdoes re-render on any search param change, even when done in a shallow manner. More research to do here.