Repository navigation
Conversation
size-limit report 📦Compared: 287ad6a (PR test merge using develop at f9f8adc) Against: ca4fc32 (develop) · base run #62950
|
2202cda to
46e486c
Compare
9069686 to
b589186
Compare
a5fcc1b to
f2a6beb
Compare
722f4c6 to
eec9ddf
Compare
|
bugbot run |
eec9ddf to
8d3dee9
Compare
|
bugbot run |
1 similar comment
|
bugbot run |
8d3dee9 to
8c6c1fa
Compare
6b24a7b to
64c909f
Compare
58c71ae to
cdbac44
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cdbac44. Configure here.
cdbac44 to
fcb441e
Compare
fcb441e to
bee8574
Compare
|
One or more gzipped bundles increased by more than 500 bytes. If this increase is intentional, add the Accept Bundlesize Increase label to this PR to rerun and accept the check. |
Optional E2E testsAll optional checks were successful. 2 successful. See the run. |
…ntry
The Vite plugin now wraps the app's server entry, so `Sentry.init({ tunnel: true })`
works with no build option and tunnel requests are answered before Start's
request middleware and routing. Every `tunnelRoute` form is served the same way:
`true` uses the DSN-derived path instead of a random one per build, and a static
path no longer patches the generated route tree.
…th Start routes on
…nel's server entry wrapper
…lides with an app route, as on develop
…and serve it from there
…ith setManagedTunnelDefaults
…equests The tunnel handler now answers a request without one with a 411.
…ants, which other variants already cover
…the client tests that repeat core's
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Some modest suggestions, but this overall is good. Replacing build-time route injection with a runtime matcher in front of the server entry is a nice improvement. Several of the comments below can be addressed by updating the PR description and changelog entry.
| const plugins: Plugin[] = [ | ||
| makeRoutePatternPlugin(), | ||
| tunnelBasePath.plugin, | ||
| makeServerEntryTunnelPlugin(options.tunnelRoute, tunnelBasePath.getBasePath), |
There was a problem hiding this comment.
This is always added, so every TanStack Start app that uses sentryTanstackStart() gets its server entry replaced by a generated module, with the original loaded through a ?sentry-tunnel-original query id. Before this PR, users without tunnelRoute got no change to their entry at all. The reason is valid, to support Sentry.init({ tunnel: true }) at runtime with no build option, but the cost falls on everyone:
- Any other Vite plugin that matches the server entry by exact id (eg with
transform(code, id)filters) now sees/src/server.ts?sentry-tunnel-originalinstead of/src/server.ts. That includes TanStack's own compiler plugins and Cloudflare's plugin. The e2e apps pass, but they cover a narrow set of setups. - The generated module adds a build-time dependency on TanStack internals for users who never asked for a tunnel. If TanStack changes the alias or default entry path, the wrapper silently stops applying, which is harmless. But if a future Start version wraps or transforms the entry differently, the query-id indirection is a new failure point.
- Per request, it adds a
getServedTunnel()lookup. That cost is small.
Suggestion: state the tradeoff in the PR body, because the description reads as "only routes matching tunnel requests".
We could also consider an opt-out (eg tunnelRoute: false to disable wrapping). At minimum, add an e2e or unit check that an app with no tunnel and a user src/server.ts with named exports still builds and serves the same as before, so that if TanStack changes something in a way that breaks us, we'll catch it early.
| }, | ||
| }); | ||
| return `import entry from ${original}; | ||
| import { wrapServerEntryWithSentryTunnel } from '@sentry/tanstackstart-react'; |
There was a problem hiding this comment.
For the default entry, the wrapper module's id is .../node_modules/@tanstack/react-start/dist/default-entry/esm/server.js. Vite resolves the bare import { wrapServerEntryWithSentryTunnel } from '@sentry/tanstackstart-react' relative to that file. @tanstack/react-start does not depend on @sentry/tanstackstart-react, so this only works because of hoisting: pnpm's default private hoist into node_modules/.pnpm/node_modules, or a flat node_modules. But in YarnPnP or pnpm with hoist=false, this can fail.
Suggestion: resolve the import from the project, not from the importer. For example, in load, call this.resolve('@sentry/tanstackstart-react', path.join(config.root, 'index.html')) (or resolve once in configResolved) and emit the resolved absolute id in the generated code. Or resolve the module from this plugin's own package.
|
|
||
| expect(response.status()).toBe(405); | ||
| expect(response.headers()['allow']).toBe('POST'); | ||
| expect(response.status()).toBe(404); |
There was a problem hiding this comment.
Hm, #24615 added the ANY handler that returned 405 Allow: POST, because without it "a non-POST (or rather GET) request to the route would cause TSS to render a page" (running root loaders). With the wrapper, only POSTs match, so a GET to /monitor falls through to Start, which SSR-renders the not-found page. The e2e test was changed from expecting 405 to expecting 404, and the test name now describes the new behavior as intended. The PR body does not mention this reversal.
Suggestion: either keep the 405 for non-POST requests on a fixed tunnelRoute path in wrapServerEntryWithSentryTunnel (the path is known there), or state in the PR body that this reverts #24615 on purpose and why. Note that for the DSN-derived path the old behavior can't hold anyway, since the path is no longer a route, so reverting may well be the right call.
| setManagedTunnelDefaults({ tunnel: tunnelRoute, basePath }); | ||
|
|
||
| // A class entry, like Cloudflare's `WorkerEntrypoint`, can't be spread into an object. The tunnel is then served | ||
| // by the wrapped default server entry it calls. | ||
| if (typeof entry === 'function') { | ||
| return entry; | ||
| } | ||
|
|
||
| return { | ||
| ...entry, | ||
| // Cloudflare Workers pass `env` and `ctx` after the request, and `withSentry` needs `ctx` to flush. | ||
| fetch(request: Request, ...rest: unknown[]) { | ||
| return handleTunnelRequestIfMatched(request) ?? entry.fetch(request, ...rest); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
{ ...entry, fetch } copies only own enumerable properties. A default export that is a class instance (methods on the prototype, such as scheduled or queue) loses those methods. That is unusual for Start, but a Worker that does export default new MyHandler() would break at runtime only for the non-fetch events.
Suggestion: add a short comment that on Cloudflare the inner wrap serves the tunnel because the client is per-request. For the spread, use Object.create(entry) with a fetch override, or Object.assign(Object.create(Object.getPrototypeOf(entry)), entry, { fetch }), so prototype methods survive.
| * to the app's server entry automatically, with its `tunnelRoute` and the base path Start routes on. | ||
| * | ||
| * `tunnelRoute` enables the managed tunnel on every server client the same way `Sentry.init({ tunnel })` does, unless | ||
| * that `init` already enabled one, and the tunnel is then served from the client's config like in every other SDK. |
There was a problem hiding this comment.
Recommend marking this @internal, and possibly exporting with a _INTERNAL_ prefix, since we're exposing some internal positional params. Or, if users should call this themselves, we could consider an options object like wrapServerEntryWithSentryTunnel(entry, { tunnelRoute, basePath })
| */ | ||
| export function makeServerEntryTunnelPlugin( | ||
| tunnelRoute: TunnelRouteOptions | undefined, | ||
| getBasePath: () => string = () => '', |
There was a problem hiding this comment.
This default only exists for tests. It'd be better to make it required, and pass in () => '' in the tests that need it.
| getBasePath: () => string = () => '', | |
| getBasePath: () => string, |
TanStack Start picked a random tunnel path per build and patched the generated route tree to serve it.
Because the new approach relies less on build-time, the logic was changed to wrap the server entrypoint instead and only route the matching tunnel requests to our handler.
This isn't a breaking change, previously we injected a single endpoint handler, now we inject a middleware since the DSN is only known at runtime.
The base path comes from
router.basepath(or Vite'sbase), and thetunnel-basepathe2e variants cover it in both dev and prod.For reviewers: if you need an overview of what this is, refer to #25199.
closes #25461