refactor(als): dedupe AsyncLocalStorage globalThis registration#1081
refactor(als): dedupe AsyncLocalStorage globalThis registration#1081james-elicx merged 1 commit intomainfrom
Conversation
Extract the `Symbol.for(...) + globalThis ??= new AsyncLocalStorage()` pattern from headers.ts, cache.ts, cache-runtime.ts, fetch-cache.ts, request-context.ts, unified-request-context.ts, and the four *-state.ts shims into a single helper at shims/internal/als-registry.ts. Cross-bundle singleton property is preserved: the helper still routes through `Symbol.for(key)` (global symbol registry) and `globalThis[sym] ??= ...`, so multiple instances of the helper module — and multiple instances of each call site — all converge on the same ALS for a given key. The helper holds no module-local reference of its own. Follow-up to #1058. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
commit: |
|
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean dedup. Reviewed all 11 files — the helper faithfully preserves the cross-bundle singleton invariant:
-
Correctness:
getOrCreateAls<T>(key)reproduces the exactSymbol.for(key) + globalThis[sym] ??= new AsyncLocalStorage<T>()semantics. The function never closes over the returned ALS — it always round-trips throughglobalThison every call. Multiple module instances ofals-registry.tsitself still hand back the same ALS for a given key. No behavioral change. -
All 12 sites converted: Verified each one in the diff:
headers.ts—vinext.nextHeadersShim.als✓cache.ts—vinext.cache.als,vinext.unstableCache.als✓cache-runtime.ts—vinext.cacheRuntime.contextAls,vinext.cacheRuntime.privateAls✓fetch-cache.ts—vinext.fetchCache.als✓request-context.ts—vinext.requestContext.als✓unified-request-context.ts—vinext.unifiedRequestContext.als✓router-state.ts—vinext.router.als✓i18n-state.ts—vinext.i18n.als✓head-state.ts—vinext.head.als✓navigation-state.ts—vinext.navigation.als✓
-
Type-only import preserved where needed:
headers.tsandunified-request-context.tscorrectly switched theirAsyncLocalStorageimport toimport typesince they only need the type for the cache-scope detection casts (as AsyncLocalStorage<unknown> | undefined), not a runtime value. The runtime construction is now fully delegated to the helper. -
Exported ALS preserved:
cache-runtime.ts'sexport const cacheContextStoragestill works —getOrCreateAlsreturns the ALS directly and the export just binds it. The cross-moduleSymbol.for("vinext.cacheRuntime.contextAls")lookup inheaders.ts(lines 188-196) still works because the symbol key is unchanged. -
Fallback patterns left inline: Correct call — the
_FALLBACK_KEY/_fallbackStatepatterns have per-module shapes (HeadState,RouterState,CacheState, etc.) that don't share structure, so they aren't candidates for the same dedup treatment. -
Module location:
shims/internal/als-registry.tsis the right spot —internal/signals this isn't part of the public shim surface, and it sits alongside the other internal helpers (cookie-serialize.ts,parse-cookie-header.ts, etc.).
CI is fully green across all checks including all Vitest shards, all 11 Playwright E2E projects, Check, CodeQL, smoke tests, and all 7 deploy previews.
Ship it.
|
Review posted. Here's the summary: Approved PR #1081 — clean dedup refactor that extracts the repeated Key findings:
|
1 participant
Summary
Extracts the repeated
Symbol.for(...) + globalThis ??= new AsyncLocalStorage<T>()pattern into a single helper,
getOrCreateAls<T>(key: string), inpackages/vinext/src/shims/internal/als-registry.ts.The pattern was inlined in 12 sites across 10 modules:
shims/headers.ts—vinext.nextHeadersShim.alsshims/cache.ts—vinext.cache.als,vinext.unstableCache.alsshims/cache-runtime.ts—vinext.cacheRuntime.contextAls(exported),vinext.cacheRuntime.privateAlsshims/fetch-cache.ts—vinext.fetchCache.alsshims/request-context.ts—vinext.requestContext.alsshims/unified-request-context.ts—vinext.unifiedRequestContext.alsshims/router-state.ts—vinext.router.alsshims/i18n-state.ts—vinext.i18n.alsshims/head-state.ts—vinext.head.alsshims/navigation-state.ts—vinext.navigation.alsThe 12 sites used identical structure: same
Symbol.for(key)registry-sidelookup, same
??=semantics, same fallback-typeAsyncLocalStorage<T>().No site initialised the ALS with a sentinel store value or wrapped it in a
container object — they were all clean candidates.
Net: 11 files changed, +77/-46 (the new helper module is the bulk of
+77; call sites collapse from 4 lines to 1).Cross-bundle singleton property — preserved
This is the load-bearing invariant for ALS bugs and is the reason the
inline pattern existed in the first place. The helper preserves it:
Symbol.for(key)consults the global symbol registry and returns thesame symbol no matter which module instance calls it.
globalThis[sym]is one slot shared by every module instance.??=only assigns when the slot is empty, so the first caller wins andevery later caller (in any module instance, in any Vite environment)
reads the same
AsyncLocalStorageinstance.The helper module itself never closes over the ALS — it always
round-trips through
globalThis. So even ifals-registry.tsis loadedunder multiple specifiers (Vite RSC/SSR/client environments, HMR), every
copy hands back the one true ALS for a given key. There is no new
"ownership-by-reference" layer introduced by the helper.
The unrelated
_FALLBACK_KEY/_fallbackStatepattern in some shims(plain-object cross-bundle singletons with shape varying per call site)
is intentionally left inline.
Files changed
packages/vinext/src/shims/internal/als-registry.ts(new)packages/vinext/src/shims/headers.tspackages/vinext/src/shims/cache.tspackages/vinext/src/shims/cache-runtime.tspackages/vinext/src/shims/fetch-cache.tspackages/vinext/src/shims/request-context.tspackages/vinext/src/shims/unified-request-context.tspackages/vinext/src/shims/router-state.tspackages/vinext/src/shims/i18n-state.tspackages/vinext/src/shims/head-state.tspackages/vinext/src/shims/navigation-state.tsFollow-up to #1058.
Test plan
pnpm fmt --writeon touched filesnpx tsc --noEmitinpackages/vinext— cleanpnpm knip— cleanpnpm vp test runforapp-router,pages-router,cache-control,cache-for-request,fetch-cache,head,request-context,unified-request-context,pages-router-concurrency— 683 tests passing. The single suite-level failure (pages-router.test.ts > allowedDevOrigins configafterAlltimeout) reproduces on cleanmainwith the changes stashed; it is a pre-existing flaky teardown unrelated to this refactor.🤖 Generated with Claude Code