fix(build): drop browser export condition for Workers SSR rendering#1849
fix(build): drop browser export condition for Workers SSR rendering#1849james-elicx wants to merge 1 commit into
Conversation
A "use client" component that imports a library which extends HTMLElement at module-init time (e.g. @number-flow/react via number-flow + esm-env) crashed Cloudflare Workers with "ReferenceError: HTMLElement is not defined". @cloudflare/vite-plugin injects `browser` into the rsc/ssr worker environments' resolve conditions, so esm-env's BROWSER flag resolves to true and the bundle evaluates `class extends HTMLElement` during SSR, where no DOM globals exist. Plain Node SSR resolves with server conditions (no `browser`), which is why `next build && next start` works. Strip `browser` from the rsc/ssr environments in configResolved so they resolve the server-safe entry, matching Node SSR. The client environment is left untouched. Adds unit tests for the helper plus a Cloudflare build regression test that asserts the server bundle resolves the non-browser entry.
commit: |
|
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
Review — PR #1849: drop
|
|
Posted the review to PR #1849. Summary of findings: The fix is correct, well-targeted, and well-tested. I verified it against the actual
Minor non-blocking notes I flagged:
Overall: LGTM. |
|
/bigbonk adversarial review for edge cases / side effects / unexpected bugs |
Problem
A
"use client"component that imports a library which registers a custom web component (extendsHTMLElement) at module-init time crashes on Cloudflare Workers with:Affected library in the report:
@number-flow/react(animated numbers). Any library that doesclass … extends HTMLElementat module load triggers it.Root cause
The crash hinges on this pattern inside
number-flow(viaesm-env):esm-env'sBROWSERresolves totrueonly under thebrowserexport condition, andfalseotherwise. So the crash requires thebrowsercondition to be active during server rendering.@cloudflare/vite-pluginsetsconditions: ["workerd", "worker", "module", "browser", …]for every worker environment — including thersc/ssrenvironments vinext renders"use client"components through. Becausebrowserprecedesdevelopment/productioninesm-env's export map,BROWSERresolves totrue, and the SSR client-reference chunk bakesconst i = HTMLElement, a = class extends i{}, which throws at module-init in workerd (no DOM globals).Plain Node SSR already resolves with server conditions (no
browser), which is exactly whynext build && next startworks — and why this only reproduces on Workers, not onvinext build && vinext start(verified: the Node SSR bundle resolvesBROWSER=falseand renders fine).Fix
In
configResolved(after the Cloudflare plugin has merged its conditions), strip thebrowserexport condition from therscandssrenvironments'resolve.conditionsand dep-optimizer conditions. SSR/RSC never run in a real browser, so isomorphic libraries now resolve their server-safe entry — matching Node SSR. Theclientenvironment is left untouched, so the browser bundle is unaffected. It is a no-op on plain Node (those environments never carrybrowser).plugins/rsc-client-shim-excludes.ts: addwithoutBrowserCondition()+stripBrowserConditionFromServerEnv()(pure, typed helpers).index.ts: call the helper for thersc/ssrenvironments.Testing
tests/use-client-html-element-build.test.ts:withoutBrowserCondition/stripBrowserConditionFromServerEnv.esm-env/number-flow/@number-flow/reactlook-alikes that share the exact resolution shape. It asserts the worker server bundle resolves the server-safe entry (BROWSER=false) while the client bundle keeps the browser entry. Fails before this change, passes after.No regressions in the affected build/SSR suites:
tsconfig-path-alias-build,prerender,build-optimization,features,app-router-production-build,app-router-production-server,app-router-worker-entry,app-router-client-preloading,app-router-rsc-plugin.vp check(format/lint/types) passes.Notes / scope
rsc/ssr), which is the reported and reproduced case. A pure Pages-Router-on-Workers app importing such a library could in theory hit the same condition — that's a separate environment-setup path and would warrant its own reproduction + follow-up.vinext build && vinext start(Node) crashing; that part does not reproduce with current vinext (Node SSR resolvesBROWSER=false). The Workers crash is the real, verifiable bug this PR fixes.