Repository navigation
Conversation
|
Bugbot run |
When wrangler `main` is a package export, such as
`"main": "vinext/server/fetch-handler"` or
`"main": "@astrojs/cloudflare/entrypoints/server"`, the Vite plugin did
not wrap the Worker. The build passed, but the bundle had no Sentry
code, and the plugin gave no warning.
`unstable_readConfig()` resolved every `main` as a path relative to the
wrangler config, so no module matched the entry. The plugin now reads
`main` the way `@cloudflare/vite-plugin` does. A value with a JS or TS
extension is a file path. Any other value is resolved with Vite, once
per environment, from the wrangler config. If it does not resolve, the
plugin warns that auto-instrumentation is off. For a package entry,
`instrument.server.*` is read from beside the wrangler config.
The resolve uses `createIdResolver` from `vite`, so `vite` is now an
optional peer dependency. `this.resolve()` in `transform` does not
work: Rollup's commonjs resolver loads each module it resolves, so a
resolve of the entry from the entry's own transform never finishes.
A `resolveId` hook does not work either, because `vite:resolve`
resolves bare specifiers before user plugins see them.
`createIdResolver` uses each environment's own conditions (for example
`workerd`), so it picks the same file that the Cloudflare plugin
bundles.
Bundled package entries often end in `export { handler as default }`.
The transform only wrapped `export default <expr>`, so it now also
wraps a default export in that form.
The new `cloudflare-autoinstrument-package-entry` e2e app installs a
local package as the Worker entry. `@sentry/astro` still needs changes
before `astro-6-cf-workers` can use a package `main`, so that app stays
as it is, and will be fixed in a subsequent PR.
Fixes #24522
Fixes JS-3725
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
90a6ab1 to
86fcec8
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 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 86fcec8. Configure here.
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Thanks for taking this. That's quite an important one
| }, | ||
| "dependencies": { | ||
| "@sentry/cloudflare": "file:../../packed/sentry-cloudflare-packed.tgz", | ||
| "worker-entry": "file:../../test-applications/cloudflare-autoinstrument-package-entry/worker-entry" |
There was a problem hiding this comment.
l: Instead of creating a new E2E test for this scenario we could rewrite existing E2E tests with frameworks that have this pattern already. E.g. in Astro we could just remove this file and change back the wrangler config to the main entrypoint of astro: @astrojs/cloudflare/entrypoints/server
wdyt?
There was a problem hiding this comment.
scratch that found this: #25347
But then I wonder if this e2e test has any additional value.
| } | ||
|
|
||
| /** | ||
| * `export { handler as default }`, the form bundlers emit for a default export. The wrapper is |
There was a problem hiding this comment.
That is a good catch. Not sure why I haven't thought about that possibility yet
| return `${wrappedName} as default`; | ||
| } | ||
|
|
||
| state.ms.append(`\nconst __SENTRY_DEFAULT_EXPORT__ = __SENTRY__.withSentry(${state.optionsFn}, ${localName});\n`); |
There was a problem hiding this comment.
l: We recently had an issue with constructor names when we re-exported durable objects (we changed the name), which got fixed with #24700. I asked my clanker to get couple of export forms that could potentially be an issue with this. It came up with this table:
It doesn't seems to be a big issue since I haven't seen any example in the wild with anonymous exports, but it is still changing the name - so maybe worth fixing (but could be in a follow up too, since the chance of hitting this is close to 0)
| } | ||
|
|
||
| // Same rule as `@cloudflare/vite-plugin`: only these extensions mark `main` as a file path. | ||
| const ENTRY_FILE_REGEX = /\.(?:m?[jt]s|[jt]sx)$/; |
This branch has not been deployed
2 participants
When wrangler
mainis a package export, such as"main": "vinext/server/fetch-handler"or"main": "@astrojs/cloudflare/entrypoints/server", the Vite plugin did not wrap the Worker. The build passed, but the bundle had no Sentry code, and the plugin gave no warning.unstable_readConfig()resolved everymainas a path relative to the wrangler config, so no module matched the entry. The plugin now readsmainthe way@cloudflare/vite-plugindoes. A value with a JS or TS extension is a file path. Any other value is resolved with Vite, once per environment, from the wrangler config. If it does not resolve, the plugin warns that auto-instrumentation is off. For a package entry,instrument.server.*is read from beside the wrangler config.The resolve uses
createIdResolverfromvite, soviteis now an optional peer dependency.this.resolve()intransformdoes not work: Rollup's commonjs resolver loads each module it resolves, so a resolve of the entry from the entry's own transform never finishes. AresolveIdhook does not work either, becausevite:resolveresolves bare specifiers before user plugins see them.createIdResolveruses each environment's own conditions (for exampleworkerd), so it picks the same file that the Cloudflare plugin bundles.Bundled package entries often end in
export { handler as default }. The transform only wrappedexport default <expr>, so it now also wraps a default export in that form.The new
cloudflare-autoinstrument-package-entrye2e app installs a local package as the Worker entry.@sentry/astrostill needs changes beforeastro-6-cf-workerscan use a packagemain, so that app stays as it is, and will be fixed in a subsequent PR.Fixes #24522
Fixes JS-3725