[APPS-2791] Fix: set ssr:true when bundling backend functions - #460
[APPS-2791] Fix: set ssr:true when bundling backend functions#460tyffical wants to merge 2 commits into
Conversation
7d48e17 to
66b0c70
Compare
66b0c70 to
d1e8071
Compare
getBaseBackendBuildConfig never set Vite's build.ssr option, so Vite defaulted to a browser-target build. Any *.backend.ts file importing a real Node builtin module (e.g. node:crypto) got that import externalized to a broken __vite-browser-external:* stub instead of a working import, breaking the bundle at build time. Backend functions run server-side, never in a browser, so ssr:true is the correct target. Affects both the existing cloud round-trip and any future local-execution path, since both share this config. Co-Authored-By: Claude <noreply@anthropic.com>
d1e8071 to
454108c
Compare
There was a problem hiding this comment.
Pull request overview
Friend, this PR configures backend functions as self-contained Vite SSR bundles, preserving Node built-in imports and inlining npm dependencies.
Changes:
- Enables SSR mode with dependency inlining.
- Adds a Node built-in regression test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/plugins/apps/src/vite/build-config.ts |
Configures standalone SSR backend bundles. |
packages/plugins/apps/src/vite/build-config.test.ts |
Tests preservation of Node built-in imports. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
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". |
The ssr.noExternal rationale described a data:-URL local-execution path that was never actually implemented — dev-server.ts's dev path builds in-memory and sends the code to the Datadog API, and the production path writes standalone temp files that get uploaded, not required from disk. Also: named the Date.now() seed instead of inlining it into the getTempWorkingDir call, and clean up the temp working directory in a finally block so repeated local/CI runs don't accumulate fixtures.
|
@codex review |
|
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". |
Motivation
getBaseBackendBuildConfignever sets Vite'sbuild.ssroption, so Vite defaults to a browser-target build for*.backend.tsbundling.import { randomBytes } from 'node:crypto') gets that import externalized to a broken__vite-browser-external:node:cryptostub instead of a working import — bundling fails with"randomBytes" is not exported by "__vite-browser-external:node:crypto".bundleBackendFunction()in the existingdev-server.tstoday, for the currentnpm run devcloud round-trip.ssr: truealone introduces a second, related issue — Vite's SSR build mode externalizes any real npm dependency it finds innode_modulesby default too, not just Node builtins, on the assumption a server runtime canrequire()it at runtime. That assumption doesn't hold here:dev-server.tswrites bundles to a standalone temp file, and this repo's ownpackages/plugins/apps/src/backend/integration.test.ts(which builds and then directlyimport()s the real emitted bundle) caught this immediately —@datadog/apps-backend/usercame back as an unresolvable bare import oncessr: truelanded. Fixed by also settingssr.noExternal: true, forcing every real dependency to stay inlined, matching the browser-mode behavior this config otherwise replaces.Changes
ssr: trueto thebuildoptions — backend functions run server-side, never in a browserpackages/plugins/apps/src/vite/build-config.tsssr: { noExternal: true }— keeps real npm dependencies inlined under SSR mode instead of externalized as unresolvable bare importspackages/plugins/apps/src/vite/build-config.tsnode:crypto, asserting the output contains a working import and no__vite-browser-externalstubpackages/plugins/apps/src/vite/build-config.test.ts(new)QA Instructions
Confirm the new test fails without the fix (for reference — already verified locally, not something you need to re-check by reverting):
Confirm the
noExternalfix specifically — this is what the real@datadog/apps-backendintegration test catches:Run the full suite:
Manual QA — real scaffolded app, local dev server + real staging cloud round-trip
Automated tests cover the bundling output; this confirms the fix through the actual
npm run devpath a customer would hit, both fixes at once (Node builtin + real npm dependency in the same function).Setup: built and
npm link'd this branch's@datadog/vite-plugin(yarn cli prepare-link+npm linkperCONTRIBUTING.md's external-project linking flow), scaffolded a fresh app (npm create @datadog/apps@latest -- test-ssr-fix --template vite-react -y), linked the local build in, added:Local (
npm run dev,POST /__dd/debugBundle): confirmed the emitted bundle contains a real workingimport { randomBytes } from 'node:crypto'(not__vite-browser-external) and@datadog/apps-backend/user'sgetExecutionUserfully inlined as real function bodies (not left as an unresolvable bare import) ✅ VERIFIEDStaging (
dd-auth --domain dd.datad0g.com -- npm run dev,POST /__dd/executeAction— realpreview-asynccloud round-trip):{"success":true,"result":{"data":{"nonce":"267d20a9","orgId":"bd4276fd-000e-11ea-a34b-3f3c8bba65b8","userEmail":"tiffany.trinh@datadoghq.com"}}}Real nonce from
node:crypto, real org/user identity from the real@datadog/apps-backendnpm dependency, executed through the actual staging cloud infrastructure end-to-end ✅ VERIFIEDBlast Radius
*.backend.tsfunction that imports a Node builtin module directly (previously silently broken at build time; now works), and any function that imports a real npm dependency (previously would have been silently externalized to an unresolvable bare import oncessr: truelanded; now stays inlined).packages/plugins/appstest suite (285 tests) — this fix specifically closes a regression the suite's ownintegration.test.tscaught against a real, unmocked bundle.ssr: true+ssr.noExternal: truetogether change Vite's target/externalization assumptions, but backend functions never ran in a browser and never relied on runtimenode_modulesresolution in the first place, so this only removes two incorrect defaults, and existing test coverage across the package (dev-server, virtual-entry, connection-collector, etc.) all still passes unchanged.Documentation