Skip to content

fix(astro): prevent sentry from externalized#9994

Merged
anonrig merged 1 commit intodevelopfrom
astro-node-prefix
Jan 2, 2024
Merged

fix(astro): prevent sentry from externalized#9994
anonrig merged 1 commit intodevelopfrom
astro-node-prefix

Conversation

@anonrig
Copy link
Copy Markdown
Contributor

@anonrig anonrig commented Dec 28, 2023 •
edited
Loading

Fixes #9777

@anonrig anonrig requested review from a team, Lms24 and lforst 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
lforst
lforst approved these changes Dec 29, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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.

lforst
lforst reviewed Dec 29, 2023
View reviewed changes
@lforst lforst mentioned this pull request Dec 29, 2023
Closed
3 tasks
@anonrig
Copy link
Copy Markdown
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
Merged
16 tasks
@billykwok
Copy link
Copy Markdown

billykwok commented Jan 1, 2024 •
edited
Loading

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
Copy Markdown
Contributor

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...

Lms24
Lms24 reviewed Jan 2, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Lms24 Lms24 left a comment •
edited
Loading

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.

@anonrig anonrig force-pushed the astro-node-prefix branch from dd338b4 to 0544181 Compare January 2, 2024 19:46
@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
@github-actions
Copy link
Copy Markdown
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%)

Lms24
Lms24 approved these changes Jan 2, 2024
View reviewed changes
Copy link
Copy Markdown
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
@anonrig anonrig deleted the astro-node-prefix branch January 2, 2024 22:34
@Lms24 Lms24 mentioned this pull request Jan 3, 2024
Merged
anonrig added a commit that referenced this pull request Jan 3, 2024
@anonrig
fix(astro): prevent sentry from externalized (#9994)
55c9b98 
Fixes #9777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lms24 Lms24 Lms24 approved these changes

@lforst lforst Awaiting requested review from lforst

+1 more reviewer

@florian-lefebvre florian-lefebvre florian-lefebvre left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Astro could not resolve X

5 participants

@anonrig @billykwok @lforst @Lms24 @florian-lefebvre