[APPS] fix(apps): build backend functions under Vite 8 / Rolldown - #468
Conversation
The Apps backend module-graph collector read `moduleInfo.ast` in `moduleParsed`. Rolldown — the bundler Vite 8 uses by default — stubs that getter to throw `UNSUPPORTED: ModuleInfo#ast`, so any app with a `.backend.ts` built under Vite 8 (or `rolldown-vite` on Vite 7) failed in `closeBundle`, after the client bundle had already succeeded. Parse `moduleInfo.code` with the plugin context's own `parse` instead. Rolldown withholds only the pre-built AST; the source and the resolved dependency IDs are both still available. Using the context's parser rather than bundling one keeps this correct by construction: the bundler already parsed this exact source to compute `importedIds`, so whatever it accepted parses here too. It also means no TypeScript-capable parser is needed, since `moduleParsed` runs after `transform` — verified against a real Vite 8.1.5 + Rolldown 1.1.5 build, where `'x' as string` arrives as `"x"` and `<div/>` arrives as a `_jsx(...)` call. No new dependency, and `this.parse` returns a node that is directly assignable, so the previous `as BaseNode` cast goes away. Filtering with `shouldTraverseCollectedModule` moves ahead of the parse. `createParsedModuleRecord` applies the same predicate, but only once an AST exists — checking first avoids parsing every `node_modules` module just to discard the result, which reading the pre-built AST never cost. Co-Authored-By: Claude <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Note: the previous review covered |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…tion' claim
Adversarial review disproved the claim that the plugin context's parse is
identical to the bundler's own. Rollup's internal module parse passes
{ jsx: this.options.jsx !== false } (rollup.js:19213) while PluginContext.parse
is bound to bare parseAst with no options (rollup.js:928), so this.parse can
throw on source the bundler accepted.
Latent today — the nested backend build never enables jsx and moduleParsed runs
post-transform — but an uncaught throw there would kill the backend build with
an opaque 'Expression expected'. Wrap it and reuse the existing
unsupportedModuleGraphDependency helper so a parser mismatch fails closed with
an actionable message, consistent with the rest of the module.
…esolutions fallback - Inject parse as a jest.fn so a test asserts the node_modules/virtual/out-of-root modules never reach the parser. Deleting the guard now fails a test instead of silently costing a parse per module. - Include the parser's own message in the fail-closed error; an actionable error that hides the syntax error is only half useful. - Branch importedIdResolutions on .length rather than nullishness. Rolldown omits the property entirely so ?? works today, but a bundler reporting it as [] with a populated importedIds would silently yield zero static dependencies.
|
@codex review Head is now
|
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Review: 'Why did this need to change?' It didn't. Reverting to the original nullish-coalescing form. The change guarded a bundler reporting importedIdResolutions as [] alongside a populated importedIds. Neither supported bundler does that: Rollup exposes a real getter (rollup.js:18382) that is only empty when importedIds is empty too, and Rolldown omits the property entirely, so both forms behave identically across the whole peer range. It was speculative hardening on an untouched context line in a PR scoped to the Rolldown fix.
sdkennedy2
changed the title
Corrected coverage table — every cell now measured before and after the fixThe earlier table only exercised the dev-server and upload paths with the fix, so nothing in it actually demonstrated the fix mattered for those paths. Filled in the missing controls. All runs against production "pre-fix" = published
Three things this establishes that the previous table did not:
Each of the four ✅ upload cells is a distinct published version in the app's history, so the runs are independently auditable. |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
2 participants
Motivation
Vite 8 switches its bundler to Rolldown, which stubs
ModuleInfo#astto throw (its AST lives in Rust memory — a deliberate, permanent API boundary):The Apps backend collector read that property to derive each backend function's
allowedConnectionIds. So any app with a.backend.tsfails to build on Vite 8 — and fails confusingly: the client bundle prints✓ built, then the plugin's nested backend build dies incloseBundlewith nothing pointing at Vite, Rolldown, or your file.@datadog/vite-plugin@3.2.8declaresvite: ">= 5.x <= 8.x", so npm installs it on Vite 8 while this documented feature is broken. Trigger is Rolldown, not the Vite major —rolldown-viteon Vite 7 hits it too.Changes
Parse
moduleInfo.codewith the plugin context's ownparseinstead of reading the bundler's pre-built AST. Rolldown withholds only the AST; the source and resolved dependency IDs are still there.One file, ~34 changed lines. Three things worth knowing while reading it:
moduleParsedruns aftertransform, so types and JSX are already compiled away — no TypeScript-capable parser needed. No lockfile or published-package changes.this.parseis the bundler's parser but not its parser configuration — Rollup binds{ jsx }to its internal parse but not toPluginContext.parse, so it can throw on source the bundler accepted. Latent today, but a module we can't parse could hide a connection ID, so it raises the existingunsupportedModuleGraphDependencyrather than an opaqueExpression expected.shouldTraverseCollectedModulemoved ahead of the parse. Reading the old AST was free; parsing isn't. Without this we'd parse everynode_modulesmodule just to discard it.The
typeof moduleInfo.code !== 'string'guard is required to compile (codeisstring | null), not defensive padding.Two alternatives implemented and rejected
@typescript-eslint/typescript-estree— eagerly requirestypescript(an optional peer), so it needed a deferredrequireto avoid draggingtypescriptinto every consumer of every published plugin. ~50 lines of loader machinery plus a dependency on all five published packages, for capability this path never exercises.parseAstexport — Vite maps itsrequirecondition to a reduced CJS entry that omitsparseAst, which would break consumers of this plugin's CJS build.QA Instructions
Verified with a real scaffolded app whose backend function calls
@datadog/action-catalog, with the connection ID in a separate module so resolution must walk the module graph. Every cell measured — pre-fix is published3.2.8, post-fix is this PR:vite build+ manifest allowlistBoth ❌ paths fail with
UNSUPPORTED: ModuleInfo#ast; pre-fix on Vite 8 the upload never reaches the network becausecloseBundlethrows first. Vite 7 unchanged throughout. Full logs, versions and method in the verification comment.Blast Radius
Only the Apps backend-function build path. The Apps plugin already refuses non-Vite bundlers, and builds without a
.backend.tsnever reach this code. No dependency changes.Risk is moderate rather than low for one reason: this collector produces a security-relevant allowlist, and silently collecting fewer connection IDs would break backend functions at runtime while the build still succeeded. Hence the fail-closed parse, the untouched specifier pairing, and QA that asserts exact collected IDs rather than exit codes.
Documentation
Not here, deliberately: a separate specifier-mispairing bug affecting Vite 6/7 today (#469, stacked); narrowing the Vite peer range (unnecessary now that Vite 8 works); adding Rolldown to the automated test matrix (repo pins
vite@6.3.5— the verification above was manual); thecreate-appsbump (different repo).🤖 Generated with Claude Code