Repository navigation
test(e2e): Add Bun, Deno and Cloudflare variants to nuxt-4 - #26285
sentry-junior[bot] wants to merge 6 commits into
Conversation
Runs the nuxt-4 suite on Node, Bun, Deno and Cloudflare from one app, following react-router-8-framework, and folds nuxt-4-cloudflare into it. Fixes the SDK defects the variants surfaced: db0 spans and prepare errors on bun-sqlite, Bun CommonJS namespaces in the orchestrion interop, and on Cloudflare the missing transaction name of request events and the tracing meta tags on cached HTML. Ref #24838 Co-Authored-By: Isaac Schlueter <isaac.schlueter@sentry.io>
size-limit report 📦Compared: bffd9a2 (PR test merge using develop at d36fbf9) Against: d36fbf9 (develop) · base run #65984 ✅ Baseline matches: this diff contains only this PR's changes.
|
Optional E2E testsSome optional checks were not successful. They do not block the merge. 7 not successful, 54 successful. See the run.
Successful checks
|
…and require the runtime variants The Vite server variant builds no server/ directory, like the other server route tests note. Bun, Deno and Cloudflare passed in CI, so they move to the required variants. The Cloudflare tunnel variant stays optional. Co-Authored-By: Isaac Schlueter <isaac.schlueter@sentry.io>
develop's addSentryPageMetaTags now handles cached pages on Node and Cloudflare, so the isCachedHtml helper from this branch is dropped. Co-Authored-By: Claude <noreply@anthropic.com>
With a server config, the module adds the sentry.server plugin on every preset. On Cloudflare, sentryCloudflareNitroPlugin already adds the page meta tags, so cached pages got two sentry-cache-origin tags.
…r too The prerenderer is built from the original config with the nitro-prerender preset, so pre-rendered pages still got both plugins' meta tags.
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
The title is test(e2e), but the PR changes released SDK behavior in @sentry/nuxt and @sentry/server-utils. The changelog comes from PR titles, so these changes would not show up there. The db.system.name change in packages/nuxt/src/runtime/utils/instrumentDatabase.ts line 77 is visible to users. Spans that had mysql2, planetscale, cloudflare-d1, better-sqlite3 or pglite now report mysql, sqlite or postgresql. That is correct by the OTel conventions, but it can break saved queries, alerts and dashboards that filter on the old values. Removing the sentry.server plugin on Cloudflare and the new transaction name on Cloudflare error events also change what users see.
Suggestion: Move the SDK changes into one or more fix(nuxt) / fix(server-utils) PRs that land first, with a changelog line for the db.system.name change. Then this PR stays a pure test(e2e) change. If splitting costs too much, at least retitle to fix(nuxt): ... and list the SDK changes in the description and the changelog.
The runtime-matrix approach is good. One app for four runtimes, with RUNTIME branches that each give a reason, is easier to maintain than separate apps. The new server tests (isolation, trace continuation, outgoing fetch propagation, logs) add coverage that had real gaps. The Bun and Deno setup follows the nextjs-16 convention and the README explains it well. The prepare-throws capture and the dialect-based db.system.name are correct fixes.
Concern 1 blocks the merge. The Cloudflare variant tests a setup that the SDK warns against, so its green result covers instrumentation that documented Cloudflare users do not get. It also seems to be the reason for the nitroServer.ts change, and maybe for the tunnel failure. Concerns 2 and 3 depend on how concern 1 is resolved. Concern 4 is a small but real correctness risk in code that runs on every runtime. It has a simple fix.
| preset: 'cloudflare_module', | ||
| cloudflare: { nodeCompat: true, deployConfig: false }, | ||
| // Sets up the SDK on Cloudflare, where the Nitro preset does not use `sentry.server.config.ts`. | ||
| plugins: ['~~/cloudflare/sentry.ts'], |
There was a problem hiding this comment.
concern 1
The Cloudflare variant builds with sentry.server.config.ts still in the app, and adds sentryCloudflareNitroPlugin on top. The documented Cloudflare setup has no server config file. The module itself warns against this combination (packages/nuxt/src/vite/addServerConfig.ts line 170: "Found sentry.server.config.ts, but the Nitro preset targets Cloudflare, where this file is not used").
The file is "not used" for init(), but its presence still changes what the module does. In packages/nuxt/src/module.ts lines 89-102, a server config file makes the module call setupNitroServer(), which adds the db0 instrumentation, storage and cache storage instrumentation, middleware instrumentation, handler-legacy, update-route-name-legacy, the sentry.server plugin and the route detector. Without a server config file (the documented Cloudflare setup), the module only adds the tunnel plugin. I checked that addDatabaseInstrumentation, addStorageInstrumentation and addMiddlewareInstrumentation have no other call sites.
So on the Cloudflare variant, the D1 db0 spans in tests/database.test.ts and tests/database-multi.test.ts, the storage spans, the middleware spans and the route names all pass because of a config the SDK tells users to remove. A user who follows the docs does not get these spans. The suite shows them as covered on Cloudflare.
This probably also explains part of defect 1 in the description. The old nuxt-4-cloudflare app had no server config and passed its tunnel test (with an /app/ base path). The new app has the extra plugins and fails. I could not confirm this without running wrangler dev, so treat it as a lead, not a finding.
The cf6542cfb1 change in packages/nuxt/src/vite/nitroServer.ts (concern 2) exists only because of this mixed setup.
Suggestion: Decide which setup the Cloudflare variant tests.
- If the documented setup (no server config) is the target, remove
sentry.server.config.tsin the Cloudflare build, for example with a Cloudflarebuild-commandthat deletes it beforenuxt build, since a runtime-specific file can replace a file but not remove it. Then skip the db0, storage and middleware tests on Cloudflare with a reason, or open an issue to add that instrumentation to the Cloudflare path. Also check whether defect 1 still happens. - If the mixed setup is meant to be supported (it does give Cloudflare users more instrumentation), then make that a deliberate SDK change in its own PR: change the warning in
addServerConfig.ts, update the docs, and keep a variant that tests the documented setup without the server config.
Either way, the PR description should say which setup the Cloudflare variant covers.
| // On Cloudflare, `sentryCloudflareNitroPlugin` adds the meta tags and captures the errors, so this plugin would add | ||
| // them a second time. | ||
| if (isCloudflarePreset(nitro.options.preset)) { | ||
| const withoutSentryServerPlugin = <T>(plugins: T[] | undefined): T[] => | ||
| (plugins ?? []).filter(plugin => plugin !== sentryServerPlugin); | ||
| nitro.options.plugins = withoutSentryServerPlugin(nitro.options.plugins); | ||
| // The prerenderer is built from the original config, with its own preset. | ||
| nitro.hooks.hook('prerender:config', config => { | ||
| config.plugins = withoutSentryServerPlugin(config.plugins); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This drops the sentry.server plugin on every Cloudflare preset, Nitro v2 and v3. On Nitro v3 (Nuxt 5), sentryCloudflareNitroPlugin returns early from its fetch branch (packages/nuxt/src/runtime/plugins/sentry-cloudflare.server.ts lines 64-95) and never hooks render:html. In that branch, the sentry.server plugin was the only source of the sentry-trace/baggage meta tags. A Nuxt 5 app on Cloudflare that has a server config file now gets no meta tags. nuxt-5-cloudflare has no server config and no meta tag test, so CI does not catch this. The unit tests in packages/nuxt/test/vite/nitroServer.test.ts only use nitro2.
Suggestion: If concern 1 ends with "no server config on Cloudflare", revert this change, because the duplicate meta tags only happen in the mixed setup. If the change stays, limit it to Nitro v2 (!isNitroV3 && isCloudflarePreset(...)) and add a nitro3 unit test.
| // export, so such a key marks a CommonJS function export on Bun. | ||
| const BROKEN_NAMESPACE_CHECK = 'Object.keys(n).length === 1'; | ||
| const FIXED_NAMESPACE_CHECK = "Object.keys(n).filter(k => k !== 'module.exports').length === 1"; | ||
| const FIXED_NAMESPACE_CHECK = |
There was a problem hiding this comment.
The new Bun check unwraps any namespace whose default is a function and that has a key matching a non-enumerable own property of that function. The comment says "No ES module namespace mirrors a non-enumerable property of its default export". That is not true. An ES module with export default function plugin() {} and export const name = 'my-plugin' has a name key, and name is a non-enumerable own property of every function. I ran the patched check on that namespace in Node, and it returns true. So a CommonJS require() of such an external now gets the bare function and loses its named exports (require(x).name gives 'plugin', not 'my-plugin'). The patch runs on every runtime, not only on Bun. The risk is small, because the module needs a function default export plus a named export called name, length or prototype. But exporting name next to a default function is a known pattern for plugin packages.
Suggestion: Also require that every other key mirrors the function, which is true of Bun's namespaces and not of the false positive above. For example: typeof n.default === 'function' && Object.keys(n).every(k => k === 'default' || k === 'module.exports' || n[k] === n.default[k]) && Object.keys(n).some(k => Object.getOwnPropertyDescriptor(n.default, k)?.enumerable === false). Add the export const name case to packages/server-utils/test/orchestrion/rollup-plugin.test.ts, and correct the comment.
| "label": "nuxt-4 (cloudflare)" | ||
| } | ||
| ], | ||
| "optionalVariants": [ |
There was a problem hiding this comment.
Deleting nuxt-4-cloudflare removes the only required Cloudflare tunnel test. Its replacement, nuxt-4 (tunnel-basepath-cloudflare), is optional and fails. Until defect 1 is fixed, a regression in the Cloudflare managed tunnel would not block a merge. The description lists this as "unsure about".
Suggestion: Look at concern 1 first, because removing the server config may make the variant pass. If it still fails, keep the old nuxt-4-cloudflare tunnel test (or a copy of it in a required nuxt-4 variant) until the fix for defect 1 lands. Do not leave the gap open. Also link the issue for defect 1 from the optional variant, so someone moves it back to required.
Why: Runs the
nuxt-4suite on Node, Bun, Deno and Cloudflare from one app, following #24598 (runtime matrix key, runtime files, README convention), and foldsnuxt-4-cloudflareinto it.@sentry/nuxtbundlessentry.server.config.tsinto the Nitro output, so Bun and Deno start.output/server/index.mjswith no preload. Their runtime config files add the@sentry/bun/@sentry/denointegrations, the same waynextjs-16does. Cloudflare builds withcloudflare_moduleand sets up the SDK withsentryCloudflareNitroPlugin. New tests cover per-request isolation, incoming trace continuation, outgoingfetchwithsentry-trace/baggage, logs, and POST errors. Where runtimes really differ, the suite branches onRUNTIMEand says why in a comment. Bun, Deno and Cloudflare were green as optional variants on the first CI run and are now required.tunnel-basepath-cloudflarestays optional (see defect 1).SDK fixes the variants surfaced:
@sentry/nuxt: db0 spans onbun-sqlite/node-sqlite(connector aliases,db.system.namefrom the dialect), and errors that eager connectors throw fromprepare.@sentry/server-utils: the orchestrion CommonJS interop unwraps Bun's function namespaces.@sentry/nuxton Cloudflare: request events get a transaction name, asnode:httpgives them on Node.Runtime differences the suite asserts as they are, not as defects: D1 has no
db.namespaceand links its errors. Worker origin isauto.http.cloudflare. The Worker build is minified. On Deno, a startup crash exits 0. The mongoose route is a stub on Cloudflare, because Nitro's Cloudflare preset can't bundle themongodbdriver even without Sentry (its optionalrequire('@aws-sdk/credential-providers')fails with "externals are not allowed").Split out, not fixed here:
nuxt-4 (tunnel-basepath-cloudflare)fail, locally and in CI).sentryCloudflareNitroPlugindecides whether to pass the body to the tunnel handler withisServedTunnelRequest()before the per-request client exists. When no client is current outside the request, as here, the body is dropped and the app answers the tunnel (401 from the auth middleware). This happens with and without a base path. Logging in the bundle showedgetClient()empty at that check, while the per-request client serves/qpo967. The oldnuxt-4-cloudflareapp happened to pass. This needs a design call on how to pick the body without a client, so it gets its own PR.sentry-transaction. The plugin reads the trace data when the request starts, before the route is known.onUnhandledRejection(modewarn) catches module-evaluation errors, which Deno routes tounhandledRejection.How to review: Start with
tests/runtime.tsand theRUNTIMEbranches, thennuxt.config.tsandcloudflare/sentry.ts. The SDK changes are small, each with unit tests.Unsure about: Cloudflare loses the required tunnel coverage
nuxt-4-cloudflarehad until defect 1 is fixed. Also, one green CI run may be too little evidence to make the runtime variants required.Ref #24838
Local runs and CI
Local runs, all on the final SDK tarballs:
test:prod: 82 passed, 3 skipped (tunnel-only tests)tunnel-basepath: 3 passed. Cloudflaretunnel-basepath: 1 passed, 2 failed (defect 1)@sentry/nuxtunit tests: 444 passed, 1 failed.test/vite/sourceMaps-nuxtHooks.test.tsfails ondeveloptoo.@sentry/server-utilsrollup plugin tests pass.pnpm run lintandlint:typespass.CI:
nuxt-4variant passes on the latest commit, including bun, deno, cloudflare and Vite server.nuxt-4 (Vite server)failed because the new server-route tests didn't skip there. The second commit fixed it.Deno Unit Testsfailed once indeno-serve.test.ts. It passes on the latest run.nuxt-4 (tunnel-basepath-cloudflare)fails with the same 401 as in defect 1.via Isaac Schlueter.
--
View Session [Sentry]