fix(shims): guard AsyncLocalStorage construction for browser bundles - #2020
Conversation
## Summary - Guard `getOrCreateAls` so it no longer calls `new AsyncLocalStorage()` unconditionally; when the runtime does not expose a usable constructor it falls back to a no-op store instead of throwing. - Add a `NoopAsyncLocalStorage` whose `getStore()` returns `undefined`, mirroring Next.js' `FakeAsyncLocalStorage` and this repo's `async-hooks-stub.ts`. - Preserve the cross-bundle singleton behavior (`Symbol.for` + `??=`). ## Root Cause In browser/client bundles `node:async_hooks` can resolve to a stub without a usable `AsyncLocalStorage` constructor (e.g. Vite's `__vite-browser-external`). `getOrCreateAls` ran `new AsyncLocalStorage()` at module-eval time, throwing `TypeError: AsyncLocalStorage is not a constructor`. Because many client-reachable shims (request-context, headers, cache, …) call this helper on import, the whole client bundle died and hydration crashed. Checking `typeof AsyncLocalStorage === "function"` before constructing, with a no-op fallback, matches Next.js' availability guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
commit: |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the change. The fix is correct and well-scoped: it guards new AsyncLocalStorage() behind a typeof === "function" check and falls back to a no-op store, matching the suggested fix in #2015, Next.js' FakeAsyncLocalStorage, and the existing async-hooks-stub.ts convention in this repo. The cross-bundle singleton (Symbol.for + ??=) is preserved and no call sites change.
Verified locally:
vp test run tests/als-registry.test.ts— 2 passedvp checkon both changed files — clean (format, lint, types)
The NoopAsyncLocalStorage implements all methods the shims actually call (getStore, run), plus enterWith/exit/disable, so client-reachable shims that take the run() path won't crash either. LGTM with a couple of non-blocking notes below.
Note on defense-in-depth: there is already an asyncHooksStubPlugin (src/plugins/async-hooks-stub.ts) that aliases node:async_hooks to a working no-op AsyncLocalStorage in the client environment. This PR is a belt-and-suspenders guard for bundling paths where that stub doesn't apply (e.g. consumers bundling the published dist/ directly, as described in #2015). That is a reasonable second line of defense — just flagging that the two no-op implementations now need to stay in sync.
| // The fallback mirrors Next.js' FakeAsyncLocalStorage: getStore() === undefined | ||
| // so shims fall back to their non-ALS code path instead of crashing. | ||
| expect(als).toBeDefined(); | ||
| expect(als!.getStore()).toBeUndefined(); |
There was a problem hiding this comment.
Minor: this assertion doesn't strongly prove the no-op fallback path was taken — the real AsyncLocalStorage also returns undefined from getStore() when no run() scope is active. The meaningful guarantee in this test is the not.toThrow() above. Consider asserting on something noop-specific to lock in behavior, e.g. that run() still invokes the callback and getStore() inside it remains undefined (a real ALS would return the store there):
| expect(als!.getStore()).toBeUndefined(); | |
| expect(als).toBeDefined(); | |
| expect(als!.getStore()).toBeUndefined(); | |
| // No-op specific: run() invokes the callback but getStore() stays undefined | |
| // (a real AsyncLocalStorage would surface the store inside run()). | |
| const seen = als!.run({ value: 1 }, () => als!.getStore()); | |
| expect(seen).toBeUndefined(); |
| * methods are best-effort no-ops that still invoke the callback. | ||
| * See: https://github.com/vercel/next.js/blob/canary/packages/next/src/server/app-render/async-local-storage.ts | ||
| */ | ||
| class NoopAsyncLocalStorage<T> { |
There was a problem hiding this comment.
Non-blocking: this NoopAsyncLocalStorage duplicates the no-op AsyncLocalStorage already defined inline in src/plugins/async-hooks-stub.ts. They are semantically identical today, but they can drift. Not necessary for this PR, but worth a follow-up to share a single source of truth (or at least cross-reference each other in a comment) so the client-stub and the registry fallback stay consistent.
|
Review posted successfully. Summary of my findings on PR #2020: Verdict: LGTM (with non-blocking notes)The fix is correct and well-scoped. It guards Strengths:
Verified locally:
Non-blocking notes posted as inline comments:
Neither note blocks merging. |
2 participants
Summary
AsyncLocalStorageconstruction in the shared ALS registry so client-reachable shims don't crash when bundled for the browser.getStore()→undefined) when no usableAsyncLocalStorageconstructor is available.Root Cause
getOrCreateAls()inshims/internal/als-registry.tscallednew AsyncLocalStorage()unconditionally. Many client-reachable shims (request-context,headers,cache, …) callgetOrCreateAls()at module-evaluation time. In browser bundles,node:async_hooksresolves to a stub without a usable constructor, sonew AsyncLocalStorage()throwsTypeError: AsyncLocalStorage is not a constructorwhile the module is evaluating — killing the whole client bundle and breaking hydration.The fix checks
typeof AsyncLocalStorage === "function"before constructing and otherwise returns aNoopAsyncLocalStoragewhosegetStore()returnsundefined, so the shims take their existing non-ALS fallback path. The cross-bundle singleton (Symbol.for+??=) is preserved and no call sites change. This mirrors Next.js'createAsyncLocalStorage()/FakeAsyncLocalStorageand follows this repo's ownasync-hooks-stub.tsconvention (no-oprunexecutes the callback rather than throwing).References
packages/next/src/server/app-render/async-local-storage.ts(createAsyncLocalStorage/FakeAsyncLocalStorage)packages/vinext/src/plugins/async-hooks-stub.tsVerification
CI=true pnpm test tests/als-registry.test.ts— new test mockingnode:async_hooksto an unusable constructor: red before (TypeError: AsyncLocalStorage is not a constructor), green after; asserts no throw,getStore() === undefined, and the same-key singleton still holds.CI=true pnpm test— full battery green (only the pre-existing env flakesdeploy.test.ts > resolveWranglerBinandoxlint-prefer-shared-utils, reproduced identically onorigin/main).CI=true npx vp check— clean on both changed files.