fix: share internal state across duplicate library copies - #1469
Conversation
Package managers can materialize several copies of the same nuqs version (pnpm keys instances by resolved peer set, and nuqs peers on next), and each copy created its own adapter context, throwing NUQS-404 for hooks living in another copy than the adapter (#798). State now lives on globalThis, keyed by version and React instance identity.
The prepack script seds the same placeholder everywhere; reusing the new version constant keeps history patching and the singleton registry aligned.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
Mutation analysis showed the sync emitter and the createContext weak key could regress without failing any test; these close that hole, and the harness now errors on partial graph leaks instead of passing vacuously.
The placeholder now feeds the cross-copy singleton keys, so a silent sed miss would collide different published versions on the same globalThis state instead of just mislabeling the NUQS-409 warning.
The 303 detection comment described the monorepo case this branch heals; it now documents what remains detectable, and the registry docblocks spell out the deliberate cross-version isolation.
Requiring react@^19 removes the 18/19 Provider shape hazard, but distinct React instances on one page still rely on this isolation; the note records what removal would trade away.
…-copies # Conflicts: # pnpm-lock.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuqs/src/adapters/next/impl.pages.ts (1)
83-111: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against synchronous throws leaving
updateState.isNuqsUpdatestuckIf
method.call()throws synchronously (before returning a promise), the.finally()never executes andupdateState.isNuqsUpdateremainstrueindefinitely. With the new shared singleton, this blocks all duplicate copies from resetting queues on future navigations — a broader blast radius than the previous local variable.🛡️ Proposed fix
updateState.isNuqsUpdate = true + try { method .call( nextRouter, { pathname: nextRouter.pathname, query: { ...urlSearchParamsToObject(search), ...urlParams } }, asPath, { scroll: options.scroll, shallow: options.shallow } ) .finally(() => { updateState.isNuqsUpdate = false }) + } catch { + updateState.isNuqsUpdate = false + throw + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nuqs/src/adapters/next/impl.pages.ts` around lines 83 - 111, Ensure updateState.isNuqsUpdate is reset even when method.call() throws synchronously: wrap the router method invocation in a try/finally (or normalize its result with Promise.resolve inside the try) so cleanup always executes, while preserving asynchronous promise handling. Update the navigation logic around method.call() and its existing finally callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nuqs/scripts/prepack.sh`:
- Around line 17-20: Replace the find-based placeholder check with a recursive
grep command using `grep -rq "0.0.0-inject-version-here" dist --include="*.js"`
in the prepack guard, preserving the existing error message and exit behavior.
In `@packages/nuqs/src/hardened-global.test.ts`:
- Around line 5-17: Ensure the nuqs test workflow builds the package before
hardened-global.test.ts imports dist/index.js. Update the relevant package test
script or task dependency so running the package tests, including the test:*
commands, invokes the build first without requiring a pre-existing dist
directory.
---
Outside diff comments:
In `@packages/nuqs/src/adapters/next/impl.pages.ts`:
- Around line 83-111: Ensure updateState.isNuqsUpdate is reset even when
method.call() throws synchronously: wrap the router method invocation in a
try/finally (or normalize its result with Promise.resolve inside the try) so
cleanup always executes, while preserving asynchronous promise handling. Update
the navigation logic around method.call() and its existing finally callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90760f52-0eb4-4795-99d9-97a84a20d26a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
packages/nuqs/package.jsonpackages/nuqs/scripts/prepack.shpackages/nuqs/src/adapters/lib/context.test.tspackages/nuqs/src/adapters/lib/context.tspackages/nuqs/src/adapters/lib/patch-history.tspackages/nuqs/src/adapters/lib/react-router.tspackages/nuqs/src/adapters/next/impl.pages.tspackages/nuqs/src/adapters/react.tspackages/nuqs/src/duplicate-copies.browser.test.tsxpackages/nuqs/src/hardened-global.test.tspackages/nuqs/src/lib/global-singleton.test.tspackages/nuqs/src/lib/global-singleton.tspackages/nuqs/src/lib/queues/debounce.tspackages/nuqs/src/lib/queues/reset.test.tspackages/nuqs/src/lib/queues/reset.tspackages/nuqs/src/lib/queues/throttle.tspackages/nuqs/src/lib/sync.tspackages/nuqs/src/lib/version.tspackages/nuqs/src/useQueryStates.tspackages/nuqs/tsconfig.jsonpackages/nuqs/vitest.config.ts
| if ! find dist -name "*.js" -exec grep -q "0.0.0-inject-version-here" {} + ; then | ||
| echo "Error: version placeholder not found in dist output" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrate the batching hazard: find splits -exec into multiple grep calls,
# and a batch without the match makes find (and the guard) fail.
mkdir -p /tmp/distcheck
printf 'const version="0.0.0-inject-version-here"\n' > /tmp/distcheck/a.js
printf 'no placeholder here\n' > /tmp/distcheck/b.js
# Force a separate invocation per file to mimic ARG_MAX splitting:
if ! find /tmp/distcheck -name "*.js" -exec grep -q "0.0.0-inject-version-here" {} \; ; then
echo "GUARD FAILED even though placeholder exists (batching hazard)"
else
echo "guard passed"
fi
# Robust alternative:
grep -rq "0.0.0-inject-version-here" /tmp/distcheck --include="*.js" && echo "grep -rq matched"Repository: 47ng/nuqs
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant script lines.
sed -n '1,80p' packages/nuqs/scripts/prepack.sh
# Behavioural probe: create enough long paths to encourage find to batch -exec.
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
mkdir -p "$tmpdir/dist"
# One file contains the placeholder; the rest do not.
printf '%s\n' 'const v="0.0.0-inject-version-here"' > "$tmpdir/dist/match.js"
# Create many long-named JS files to make argument batching more likely.
python3 - <<'PY' "$tmpdir/dist"
import os, sys
dist = sys.argv[1]
name = "x" * 180 + ".js"
for i in range(2500):
with open(os.path.join(dist, f"{i:04d}-{name}"), "w") as f:
f.write("no match\n")
PY
# Show how many invocations find makes and whether the guard can fail.
count=0
if ! find "$tmpdir/dist" -name "*.js" -exec sh -c '
count_file="$1/.count"
n=0
[ -f "$count_file" ] && n="$(cat "$count_file")"
n=$((n+1))
printf "%s\n" "$n" > "$count_file"
exec grep -q "0.0.0-inject-version-here" "$@"
' sh "$tmpdir/dist" {} +; then
echo "guard failed"
else
echo "guard passed"
fi
echo "invocations: $(cat "$tmpdir/dist/.count" 2>/dev/null || echo 0)"Repository: 47ng/nuqs
Length of output: 1341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
mkdir -p "$tmpdir/dist"
# One matching file, many non-matching JS files to encourage batching.
printf '%s\n' 'const v="0.0.0-inject-version-here"' > "$tmpdir/dist/match.js"
for i in $(seq 1 3000); do
printf '%s\n' "no match $i" > "$tmpdir/dist/$(printf '%04d' "$i").js"
done
# Count how many times find invokes the command and record whether any batch matches.
count_file="$tmpdir/count"
touch "$count_file"
if find "$tmpdir/dist" -name "*.js" -exec sh -c '
count_file="$1"
shift
n=$(cat "$count_file")
n=$((n + 1))
printf "%s\n" "$n" > "$count_file"
grep -q "0.0.0-inject-version-here" "$@"
' sh "$count_file" {} +; then
echo "find exit: 0"
else
echo "find exit: non-zero"
fi
echo "invocations: $(cat "$count_file")"Repository: 47ng/nuqs
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!**/node_modules/**' '0\.0\.0-inject-version-here|globalThis singleton keys|singleton keys' .Repository: 47ng/nuqs
Length of output: 712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
mkdir -p "$tmpdir/dist"
# One matching file and many long non-matching JS files to force batching.
printf '%s\n' 'const v="0.0.0-inject-version-here"' > "$tmpdir/dist/0000-match.js"
longname="$(printf 'x%.0s' {1..180})"
for i in {1..4000}; do
: > "$tmpdir/dist/$(printf '%04d' "$i")-$longname.js"
done
count_file="$tmpdir/count"
if find "$tmpdir/dist" -name "*.js" -exec sh -c '
count_file="$1"
shift
n=0
[ -f "$count_file" ] && n="$(cat "$count_file")"
n=$((n + 1))
printf "%s\n" "$n" > "$count_file"
grep -q "0.0.0-inject-version-here" "$@"
' sh "$count_file" {} +; then
echo "find exit: 0"
else
echo "find exit: non-zero"
fi
echo "invocations: $(cat "$count_file" 2>/dev/null || echo 0)"Repository: 47ng/nuqs
Length of output: 182
Guard can fail spuriously when find splits -exec across batches. grep -q returns 1 for any batch that doesn’t contain the placeholder, and find propagates that non-zero status even if another batch matched. A larger dist tree can therefore break this release check; use grep -rq "0.0.0-inject-version-here" dist --include="*.js" instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nuqs/scripts/prepack.sh` around lines 17 - 20, Replace the
find-based placeholder check with a recursive grep command using `grep -rq
"0.0.0-inject-version-here" dist --include="*.js"` in the prepack guard,
preserving the existing error message and exit behavior.
|
🧪 This PR is included in nuqs@2.9.1-beta.2 The release is available on:
|
|
🚀 This PR is included in nuqs@2.9.1 The release is available on: |
Monorepos can load multiple physical copies of the same nuqs version, especially when pnpm resolves different peer sets. Previously, each copy created its own adapter context and update queues, so hooks from one copy could not see an adapter from another and threw NUQS-404.
This PR moves internal mutable state to a versioned
globalThisregistry. Same-version copies now share:Different nuqs versions remain isolated. In environments where
globalThisis non-extensible, nuqs falls back to module-local state instead of failing during import.The duplicate-copy test harness loads a real second source graph and covers context sharing, optimistic updates, batching, debounce behavior, external History API updates, and Pages Router queue safety. A Node test also covers hardened realms.
Verified with the full test suite, a packed pnpm workspace reproduction, and Next.js Turbopack.
No public API or type changes.
Closes #798.