fix(transforms): resolve server-side esm.sh modules with a server target - #3541
Conversation
The SSR/SSG HTTP module cache resolved bare npm specifiers with target=es2022. esm.sh applies the `browser` export condition for browser targets, so packages that ship a DOM implementation behind it resolve to one and then throw when evaluated server-side. `veryfront build` on the default ai-agent template failed with "ReferenceError: document is not defined": react-markdown and remark-gfm both pull decode-named-character-reference, whose browser build calls document.createElement at module scope. Resolve bare specifiers for this cache with target=denonext. React URLs are resolved earlier in the same function and are unchanged, as is the normalizeEsmShUrl default, which also canonicalizes browser-facing release-asset URLs. Scope the leading /denonext/ strip to the path prefix so it no longer rewrites an inner segment of a resolved build path.
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ESM URL helpers now use the ChangesESM URL resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e778fc6ca3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This cache exists to keep SSR runtime-agnostic across Deno, Node, and Bun (see transforms/pipeline/stages/ssr-http-cache.ts). denonext selects esm.sh's deno condition, which can resolve Deno-only APIs into a module that is then evaluated under Node or Bun. node is the portable server condition and is equally free of the browser DOM builds this fix targets.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/transforms/esm/http-cache-helpers.test.ts (1)
19-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the required target independently.
The resolver and these expectations both read
SERVER_ESM_TARGET. If that constant changes toes2022, the implementation and expected values can change together while the test remains green.Use the literal target in the regression expectation, or add an independent assertion that
SERVER_ESM_TARGETequals"denonext".Proposed test guard
for (const specifier of ["react-markdown@9.0.3", "remark-gfm@4.0.1"]) { const result = resolveBareSpecifier(specifier, emptyImportMap); assertEquals(result.includes("target=es2022"), false, specifier); - assertEquals(result.includes(`target=${SERVER_ESM_TARGET}`), true, specifier); + assertEquals(result.includes("target=denonext"), true, specifier); }The PR objective requires the exact
denonextserver target.Also applies to: 746-764
🤖 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 `@src/transforms/esm/http-cache-helpers.test.ts` at line 19, Update the expectations in the ESM transform tests to independently enforce the literal "denonext" server target rather than deriving it solely from SERVER_ESM_TARGET. Add an assertion that SERVER_ESM_TARGET equals "denonext" and ensure the related cases around the resolver expectations retain this exact target.
🤖 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 `@src/transforms/esm/http-cache-helpers.ts`:
- Around line 548-568: Extend the focused tests for normalizeHttpUrl to cover an
esm.sh URL whose path contains an inner /denonext/ segment, such as
/pkg@1/X-abc/denonext/pkg.mjs. Assert that normalization preserves this path
unchanged, while retaining the existing leading-segment removal coverage.
---
Nitpick comments:
In `@src/transforms/esm/http-cache-helpers.test.ts`:
- Line 19: Update the expectations in the ESM transform tests to independently
enforce the literal "denonext" server target rather than deriving it solely from
SERVER_ESM_TARGET. Add an assertion that SERVER_ESM_TARGET equals "denonext" and
ensure the related cases around the resolver expectations retain this exact
target.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b645d44f-28c5-4e1c-929a-3025af8c09fa
📒 Files selected for processing (2)
src/transforms/esm/http-cache-helpers.test.tssrc/transforms/esm/http-cache-helpers.ts
Review: correct fix, verified end to end, incomplete in one placeI re-derived everything below rather than reusing the PR's evidence. Independent verificationPremise, against esm.sh directly: A/B, pristine
The output is real: 12.7 KB body, hydration data, SSR head, page island, CSS asset. The description undersells the changeScoping the That FindingsP1 — the same defect survives one function away. Reproduced on this branch: switch the template's two imports to P2 — two more unscoped
Both still do P2 — the new regression test asserts a specifier the build never produces. The test pins That version drop is pre-existing ( P3 — the cache key now misreports the target. Server cache after a clean build: 92 Checked and clear: Practical caveat
RecommendationMerge. Fold in the |
|
Addressed the remaining review feedback in 40641ca. The target-independence assertion uses the current runtime-neutral literal node (the earlier CodeRabbit nitpick referenced the superseded denonext commit), and the focused inner-path regression now preserves nested /denonext/ segments. Focused verification passed 76 steps, plus format and diff checks. |
|
Full template matrix. Every template scaffolded fresh and built,
5 of 7 templates are broken on main; all 7 pass here. Wider than the default template alone — I had only verified The two that already passed ( Outputs verified non-hollow, since
Each dist is 448–516 KB with real SSR markup, so these are genuine builds rather than empty passes. |
Ships the build fixes for veryfront-issue-inbox#456. `veryfront build` has been broken for every project since 0.1.1206, and 5 of 7 scaffold templates could not build at all. - #3538 prime host contracts in the build extension path - #3541 resolve server-side esm.sh modules with a server target - #3542 keep optional Deno imports optional - #3540 isolate cwd-mutating unit tests The version is embedded in hydration-runtime.generated.ts, so a bump must be followed by `deno task generate`; the manifests check fails otherwise. There is no earlier version to pin to: 0.1.1205 fails the same templates with the CSSProcessor error that #3417 was fixing.
Follows #3538 (now merged). That PR fixed the extension-contract error; this fixes the failure hiding behind it, so
veryfront init→veryfront buildworks end to end.Related: veryfront-issue-inbox#456.
Problem
With #3538 merged, the default
ai-agenttemplate still failed to build:ai-agentis the default template (cli/commands/init/catalog.ts:24), so a bareveryfront initproduced a project that could not build.Cause
The SSR/SSG HTTP module cache resolved bare npm specifiers with
target=es2022. esm.sh applies thebrowserexport condition for browser targets, so a package that ships a DOM implementation behind that condition resolves to one — and then throws when the module is evaluated server-side.The template's
markdown-renderer.tsximportsreact-markdownandremark-gfm. Both pulldecode-named-character-reference, whose browser build is:Confirmed against esm.sh directly:
es2022document.createElementdenonext/nodedocumentrefsFix
Resolve bare specifiers in this cache with
target=denonext.Scope notes, since this file is shared:
resolveBareSpecifierand are untouched, so SSR React singleton identity is unchanged.normalizeEsmShUrldefault stayses2022. It also canonicalizes browser-facing URLs viasrc/release-assets/build-executor.ts; changing it there would ship Deno-targeted builds to browsers. I tried that first and backed it out./denonext/strip is now scoped to the path prefix. It previously used a substring replace, which would also rewrite an inner/denonext/segment of a resolved build path. The existing test covers the prefix form (esm.sh/denonext/lodash@4) and still passes.Only
specifier-resolver.tscallsresolveBareSpecifier, and it writesfile://modules for the SSR runtime, so this path is server-only.Verification
A/B on the single file, default template, pristine scaffold, cache cleared each run:
Output is real, not a hollow pass: 14.7 KB
index.htmlwith the Chat component's SSR markup, import map, hydration runtime,client.js/app.js, and a CSS asset.deno task typecheck— exit 0deno test src/transforms/— 154 passed, 0 faileddeno test src/release-assets/ src/rendering/ src/build/ src/discovery/— 216 passed, 0 faileddeno fmt --check+deno linton both files — cleanThree existing assertions encoded the old browser target and were updated. Added a regression test naming the two template packages.
Follow-ups, not in this PR
src/build/production-build/static-generation.ts:413). The cause is preserved but never surfaced, which made this materially harder to isolate.cli/commands/build/build-error.integration.test.ts:16-18is anassertRejectswith no error type or message constraint, so it passes whichever way the build fails. It stayed green through both this bug and the contract regression.Summary by CodeRabbit
New Features
Bug Fixes