fix(toolkit): prevent infinite refetch loop when query args contain NaN - #5339
Open
aagostino31 wants to merge 1 commit into
Open
fix(toolkit): prevent infinite refetch loop when query args contain NaN#5339aagostino31 wants to merge 1 commit into
NaN#5339aagostino31 wants to merge 1 commit into
Conversation
copyWithStructuralSharing compared values with ===, so NaN never equaled itself and an arg object containing NaN got a new identity on every render. useQuerySubscription treats that as an arg change and dispatches initiate again. While the query is in an error state the thunk condition allows the dispatch through, so every rejection triggers a rerender which dispatches a new request, refetching in a loop for as long as the hook is mounted. Comparing with Object.is treats NaN as equal to itself and restores a stable arg identity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
aryaemami59
changed the title
NaN
@reduxjs/rtk-codemods
@rtk-query/codegen-openapi
@rtk-query/graphql-request-base-query
@reduxjs/toolkit
commit: |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #5338
AI warning - I certainly fixed this with Claude, but I did go through a lot of manual testing and verification for this.
The problem
If the argument passed to a
useQueryhook containsNaNand the request returns an error, the hook dispatches the same query again in an endless loop. Each rejection causes a rerender and each rerender dispatches a new request, so the loop runs for as long as the hook is mounted, paced only by network latency.This is easy to hit in practice. Our case was a URL where a route param placeholder was never substituted, so the page received the literal string
"worker_id", andNumber("worker_id")producedNaNwhich then flowed into a query arg.The regression was introduced in 2.9.0 when
useStableQueryArgsswitched from comparing serialized args to reference checks viacopyWithStructuralSharing. On earlier versions the same code fails once and stops.The cause
copyWithStructuralSharingdecides whether the new object is identical to the old one using strict equality:NaN === NaNisfalse, so an arg object containingNaNis never considered equal to the previous one anduseStableQueryArgsreturns a new object on every render.useQuerySubscriptionthen seeslastPromise.arg !== stableArgon every render and dispatchesinitiateagain. While the query is in an error state, the thunkconditionlets the dispatch through because the cache entry has nofulfilledTimeStamp, so every one of these dispatches reaches the network:The serialized cache key is unaffected the whole time, since
JSON.stringifyturnsNaNintonull, so all of this happens on a single cache entry.The fix
Compare values with
Object.is, which treatsNaNas equal to itself. One line incopyWithStructuralSharing.Tests
copyWithStructuralSharing: objects containingNaNpreserve identity when equal, and unchanged branches containingNaNare still shared when a sibling value changes.NaNand a bareNaNarg, each asserting the base query runs exactly once when the query errors. Before the fix the object case makes 27 requests in 150ms against a 5ms mock; after the fix it makes exactly one.The bare
NaNcase already behaved correctly before this change because React compares effect dependencies withObject.is, so a primitiveNaNdep never retriggers the subscription effect. The test is included to lock that behavior in.No public types change, so the API report is untouched.
yarn testfor the toolkit package passes: 88 files, 1320 tests, no type errors.🤖 Generated with Claude Code