fix(context): share AsyncLocalStorage across ESM+CJS variants via globalThis#1779
fix(context): share AsyncLocalStorage across ESM+CJS variants via globalThis#1779bellcoTech wants to merge 1 commit into
Conversation
…balThis `@cedarjs/context` ships dual ESM+CJS and Node resolves the variant per caller. In a mixed-variant load (e.g. an ESM user-api package alongside cedar tooling that loads `@cedarjs/api-server`'s CJS entry), each variant is a separate module instance with its own module-scoped state. The `AsyncLocalStorage` singleton previously lived in a module-scoped `let`, so each variant had its own store. The framework's request handler would write `context.currentUser` into the variant it loaded while user code reading `context.currentUser` saw the other variant's empty store — a textbook dual-package hazard. Concretely this surfaces as every `requireAuth`-gated query failing with "You do not have permission to do that" even after `getCurrentUser` runs successfully, because the user written to the framework's CJS store is invisible to the user code reading from the ESM store (or vice versa). Stashing the singleton on `globalThis` under a `Symbol.for` key bridges the two variants: both resolve the same key to the same object and so share the store regardless of which variant either loaded. The key is namespaced (`__cedarjs_context_storage__`) to avoid any chance of collision. No behaviour change for single-variant projects — they previously got one module instance and one store, and they still get one module instance and one store, just rooted on `globalThis` instead of in a module-scoped `let`. The fix is purely defensive against the dual-package case.
👷 Deploy request for cedarjs pending review.Visit the deploys page to approve it
|
|
@Tobbe i've done this PR and the other PR for a couple of friction points I had getting to 4.2. This one I couldn't get around, but the other I could so not as important. |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many -t build:pack --exclude create-ceda... |
✅ Succeeded | 2s | View ↗ |
nx run-many -t test --minWorkers=1 --maxWorkers=4 |
✅ Succeeded | 1m 56s | View ↗ |
nx run-many -t build |
✅ Succeeded | 5s | View ↗ |
nx run-many -t test:types |
✅ Succeeded | 7s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-05-14 13:42:46 UTC
Greptile SummaryThis PR fixes the dual-package hazard in
Confidence Score: 3/5The runtime fix is correct, but the TypeScript type of the return value may expose callers to a compile error under strict null checks before this can land cleanly. The ContextStorageGlobal type marks the [STORAGE_KEY] property as optional (?:), so g[STORAGE_KEY] carries an | undefined type. TypeScript control-flow narrowing does not reliably strip undefined from computed symbol-indexed properties on mutable globals, meaning getAsyncStoreInstance() likely infers an AsyncLocalStorage | undefined return type. All three call-sites in context.ts chain .getStore() directly on the return value, which would be a compile error that needs resolving before the PR is merge-ready. packages/context/src/store.ts — specifically the return statement of getAsyncStoreInstance and whether tsc passes with strict null checks enabled Important Files Changed
Reviews (1): Last reviewed commit: "fix(context): share AsyncLocalStorage ac..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return g[STORAGE_KEY] | ||
| } |
There was a problem hiding this comment.
g[STORAGE_KEY] is typed as AsyncLocalStorage<Map<string, GlobalContext>> | undefined because the ContextStorageGlobal type marks the property optional with ?:. With strict: true (and thus strictNullChecks) in the repo's base tsconfig, TypeScript's control flow analysis does not reliably narrow computed symbol-indexed properties on mutable objects like globalThis across an if-assignment boundary — so the inferred return type of getAsyncStoreInstance likely includes undefined. Every caller in context.ts chains .getStore() directly onto the return value without a null check, which would be a compile-time error under strict null checks. A non-null assertion on the return makes the invariant explicit and preserves the original AsyncLocalStorage (never-undefined) return type.
| return g[STORAGE_KEY] | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |
| return g[STORAGE_KEY]! | |
| } |
|
The context package has been a constant source of ESM+CJS interop issues :( I thought I had them all fixed, but apparently not. Can you please explain again how to reproduce the issue? |
|
Hey @Tobbe I think I've got a way to reproduce it — looks like context doesn't cross the CJS ↔ ESM line inside I first thought it was my own setup — I had Attached a script that scaffolds a fresh ESM project and runs a quick test loading the ESM and CJS variants of I think the reason a bare template doesn't show it: |
|
@bellcoTech Did start failing for you between 4.1 and 4.2? Does it only happen with ESM Cedar apps? const esmStore = await import('@cedarjs/context/dist/store')
const cjsStore = require('@cedarjs/context/dist/store.js')I understand that two different contexts will be loaded by that code. And I understand that your patch fixes it, and I understand why it fixes it. But I still don't understand how this happens in an actual Cedar app. Did you really explicitly import |
|
I tried to have AI reproduce. Here's what I found after doing this properly with the actual Cedar server and curl: The dual-package hazard doesn't manifest on Node 24 with the current dev server. I set up the exact scenario — CJS-root project, The reason: Node 24 supports The PR #1779 fix is still valuable, but it's a defensive measure for environments where CJS It's not that I don't believe you ran into this issue (like I said, I've seen it plenty of times myself). It's just that I don't know how to reproduce properly with the current code |
|
Sorry for the delay - timezones and all. |
|
Used AI to dig further. Here's what it came back with after a verified end-to-end reproduction in a vanilla cedar 4.2 project: The variant split is controlled by cedar's CLI bin selection. From if (workspace.includes("api")) {
const isEsm = rootPackageJson.type === "module"
const serverWatchCommand = isEsm ? `cedarjs-api-server-watch` : `cedar-api-server-watch`
const cedarConfigPath = getConfigPath()
jobs.push({
name: "api",
command: [...],
})
}The check is purely on the root "bin": {
"cedar-server": "./dist/cjs/bin.js",
"cedar-api-server-watch": "./dist/cjs/watch.js",
"cedarjs-server": "./dist/bin.js",
"cedarjs-api-server-watch": "./dist/watch.js"
}cedar- prefixed bins → When root type !== "module" User code in api/src/... (built to api/dist/...) loads as ESM When root type === "module" # 1. Scaffold a fresh ESM cedar 4.2 project
yarn create cedar-app --typescript --esm --no-git-init --no-install --yes /tmp/cedar-repro
cd /tmp/cedar-repro && yarn install
# 2. Wire up real auth (any provider — dbAuth is simplest, no external services)
yarn cedar setup auth dbAuth --force --no-webauthn --createUserModel --no-generateAuthPages
yarn cedar prisma migrate dev --name init
yarn cedar generate dbAuth --force --no-webauthn --username-label Email --password-label Password
# 3. Add a Post model + scaffold (auto-gated with @requireAuth)
cat >> api/db/schema.prisma <<'EOF'
model Post {
id Int @id @default(autoincrement())
title String
body String
createdAt DateTime @default(now())
}
EOF
yarn cedar prisma migrate dev --name add-post
yarn cedar generate scaffold post --force
# 4. THE TRIGGER: remove "type": "module" from the root package.json
# (the esm-ts template ships with it; removing simulates a project where
# root and api workspace types disagree)
node -e "const f='./package.json';const p=require(f);delete p.type;require('fs').writeFileSync(f,JSON.stringify(p,null,2)+'\n')"
# 5. Run dev, open in incognito (avoid any leftover auth cookies)
yarn cedar devThen in incognito: http://localhost:8910/signup — sign up with any email + password The earlier repro probably stayed single-variant because the test project had root type: module (the esm-ts template default), so cedar picked the ESM bin chain and there was no split to begin with. The CJS-bin path only activates when root and api workspace types disagree. Scope Worth considering on cedar's side: a CLI warning when the root/api type mismatch is detected, or a docs note that the two should agree. So it looks like I missed a step with the root package.json type assuming the API package.json would suffice. Took a bit to track down but this all lines up with my project and I was able to reproduce. |
Thank you! Yes, that'd do it. I only test for everything being CJS or everything being ESM (across all three package.json files). Not a mixture. A warning somewhere in the cedar cli, like your AI agent suggests, makes a lot of sense |
|
Ok great, I didn't completely waste your time 😅😅. Would you like me to do the warning? Or leave it for you? |
|
Please, if you can spare the time, go ahead with the warning. Thanks! |
|
Sure more than happy to |
2 participants
@cedarjs/contextships dual ESM+CJS and Node resolves the variant per caller. In a mixed-variant load (e.g. an ESM user-api package alongside cedar tooling that loads@cedarjs/api-server's CJS entry), each variant is a separate module instance with its own module-scoped state.The
AsyncLocalStoragesingleton previously lived in a module-scopedlet, so each variant had its own store. The framework's request handler writescontext.currentUserinto the variant it loaded; user code readingcontext.currentUsersees a different variant's empty store. EveryrequireAuth-gated query then fails with "You do not have permission to do that" even thoughgetCurrentUserran successfully — the user written to the framework's CJS store is invisible to user code reading from the ESM store (or vice versa).Puts the singleton on
globalThisunder aSymbol.forkey so both variants resolve to the same slot and share one store. Key is namespaced (__cedarjs_context_storage__).No behaviour change for single-variant projects — they had one module instance and one store before, they have one module instance and one store after, just on
globalThisinstead of a module-scopedlet.The package doesn't have a test suite today, so I haven't added one. Happy to wire up vitest and add coverage if you'd like.