fix(react): don't call useRef conditionally in checkQueryChanged - #1069
Open
giaBaoJS wants to merge 1 commit into
Open
fix(react): don't call useRef conditionally in checkQueryChanged#1069giaBaoJS wants to merge 1 commit into
giaBaoJS wants to merge 1 commit into
Conversation
`checkQueryChanged` declared `React.useRef` after an early `return` in the `query.compile()` catch block. A query which throws while being compiled on one render but not the next therefore changes the order of hooks, which crashes the component. Move the ref above the compilation and record failed compilations, so that a query which compiles again is re-applied to the consumers of the hook.
🦋 Changeset detectedLatest commit: 5fe853d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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
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.
Summary
useQueryhard-crashes when the query it is given throws while being compiled on one render but not on the next.checkQueryChangeddeclaresReact.useRefon line 29, after the earlyreturn falseon line 22:The hook is therefore called conditionally, which breaks the rules of hooks.
checkQueryChangedruns on everyuseQuery,useSuspenseQueryanduseSingleSuspenseQueryrender throughconstructCompatibleQuery, so the whole component tears down:React reuses the value of the hook that previously occupied that slot, so
previousQueryRef.currentisundefinedand the very next line throws.It breaks in both directions. When a query that used to compile starts failing, the render is one hook short instead:
How this is reached
compile()throwing is supported behaviour, not misuse: the suite already covers it inshould show an error if parsing the query results in an error. That test survives only because itscompile()throws on every render, so the hook count stays consistent. The crash needs the outcome to change between renders.The realistic trigger is a conditionally built projection. With
drizzle-orm@0.44.7(the version this repo pins) andtoCompilableQuery:so a component that builds its projection from optional props/state —
select({ id: lists.id, name: showName ? lists.name : undefined })— compiles on some renders and throws on others, and togglingshowNamecrashes the tree. I checked the other shapes I expected to throw (inArray(col, []),eq(col, undefined),limit(NaN)) and they all compile fine, so the trigger is genuinely narrow — but the failure mode when it is hit is an unrecoverable crash rather than the error state the hook is designed to report.This survived because
react-hooks/rules-of-hooksnever ran here. The rootpackage.jsonhas"lint": "eslint .", but there is no eslint config and no eslint (oreslint-plugin-react-hooks) dependency at the root, sopnpm lintcannot run.The fix
Declare the ref before compiling, and let it hold three states:
undefined— initial render, nothing observed yetnull— the previous render could not compile the queryBehaviour for a query that always compiles is unchanged: the initial render still reports "not changed" (previously achieved by seeding the ref with the current values), and later renders still report a change only when the SQL, the parameters or the options differ.
One behaviour is deliberately new. When a compilation failure is followed by a successful one,
checkQueryChangednow returnstrue. Simply hoisting the ref and treating the first successful compile as "not changed" stops the crash but leaves the hook broken in a quieter way: theWatchedQuerywas created with the query that could not be compiled,useWatchedQueryonly propagates a new query throughupdateSettingswhenqueryChangedis true, so the hook would stay stuck onerrorforever. I verified that — with the hoist-only variant, the addeduseQuerytest fails withexpected Error: error to be falsy. Recording the failed compilation is what lets consumers pick the query up again.queryChangedhas three consumers and the change is consistent with all of them:useWatchedQuery—truecallswatchedQuery.updateSettings({ query, ... })inlineuseSingleQuery—queryChangedsits in theuseCallback/useEffectdeps, so flipping it re-runs the queryuseSuspenseQuery/useSingleSuspenseQuery— ignore the value, but still go throughconstructCompatibleQueryand so were exposed to the same crashTests
packages/react/tests/useQuery.test.tsx— auseQuerytest next to the existing "parsing the query results in an error" one, asserting that a query which starts compiling after a failed compilation neither crashes nor stays in the error state. It runs in both the normal andStrictModevariants of the existing matrix.packages/react/tests/watchUtils.test.tsx— unit tests forcheckQueryChangedcovering both crash directions plus the unchanged semantics (initial render, unchanged query, changed SQL, changed parameters, changed options, permanently failing compilation). The six semantics tests pass before and after the change, which is what pins the existing behaviour down.Before:
82 passed | 2 skipped (84).After:
92 passed | 2 skipped (94).With the change to
watch-utils.tsreverted and the new tests kept, 4 of them fail with theTypeError/Rendered fewer hooks than expectedabove, and the 6 semantics tests still pass.common,drizzle-driver,kysely-driver,tanstack-react-query,vueandwebare green.