Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(astro): prevent sentry from externalized #9994

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Conversation

anonrig
Copy link
Contributor

@anonrig anonrig commented Dec 28, 2023

Fixes #9777

@anonrig anonrig requested review from a team, lforst and Lms24 and removed request for a team December 28, 2023 21:49
@anonrig anonrig force-pushed the astro-node-prefix branch 5 times, most recently from a184ecf to fcb1fd5 Compare December 29, 2023 00:24
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks good to me. I want @Lms24 to make the last call before merging this because I lack a lot of context on the Astro SDK.

packages/astro/src/integration/namespace.ts Outdated Show resolved Hide resolved
packages/astro/src/integration/namespace.ts Outdated Show resolved Hide resolved
@anonrig
Copy link
Contributor Author

anonrig commented Dec 29, 2023

This generally looks good to me. I want @Lms24 to make the last call before merging this because I lack a lot of context on the Astro SDK.

I'm happy to wait @Lms24 to land this.

@florian-lefebvre florian-lefebvre mentioned this pull request Dec 30, 2023
16 tasks
@billykwok
Copy link

billykwok commented Jan 1, 2024

Just applied the changes to my repo and it doesn't seem to be working.

AFAIK, astro treats @sentry/ as external dependencies, so all dependencies are pre-bundled using esbuild without going through rollup plugins. As a result, the resolveId(id, importer) hook is not run for the import and require statements of dependencies (in this case @sentry/node). You might need to mark @sentry/ as external first.

You can verify this by printing out the id argument in the hook. Let me know if I'm missing something.

@lforst
Copy link
Member

lforst commented Jan 2, 2024

@billykwok I thought vite was using esbuild only as a transpiler. Otherwise any rollup/vite plugins would have a hard time working...

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the change looks good, generally. +1 on trying to limit the id modification to our @sentry/astro package.

However, I'm a bit unsure if this is enough to get the server side working with CF pages. None of our Fullstack SDKs are compatible with edge/WinterCG runtimes (except for NextJs with Vervel Edge), which I believe also comes down to our general problems with ESM support and the require statements in our http integration and Node transport. I guess we can try though!

One request: Let's guard adding the plugin similarly to how we guard adding the Sentry Vite plugin (sourcemaps) after introducing the enabled option in #10007. No need to add the plugin if Sentry is disabled.

packages/astro/src/integration/namespace.ts Outdated Show resolved Hide resolved
@anonrig anonrig changed the title fix(astro): handle builtin modules without node prefix fix(astro): prevent sentry from externalized Jan 2, 2024
@anonrig anonrig requested review from Lms24 and lforst January 2, 2024 19:46
Copy link
Contributor

github-actions bot commented Jan 2, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 75.94 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 67.3 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 60.93 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.88 KB (+0.06% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 30.42 KB (0%)
@sentry/browser - Webpack (gzipped) 22.12 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 73.34 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 65.01 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 31.16 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 23.16 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 203.92 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 93.81 KB (+0.05% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 68.73 KB (+0.07% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 34.12 KB (+0.03% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 67.71 KB (+0.02% 🔺)
@sentry/react - Webpack (gzipped) 22.16 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 84.35 KB (+0.02% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 48.99 KB (+0.03% 🔺)
@sentry-internal/feedback - Webpack (gzipped) 16.63 KB (0%)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@anonrig anonrig merged commit d2e0465 into develop Jan 2, 2024
32 of 33 checks passed
@anonrig anonrig deleted the astro-node-prefix branch January 2, 2024 22:34
anonrig added a commit that referenced this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Astro could not resolve X
5 participants