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(nextjs): Don't accidentally trigger static generation bailout #9749

Merged
merged 3 commits into from Dec 5, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Dec 5, 2023

We are running into an issue where we are triggering static generation bailout because we are accessing dynamic objects (e.g. the Request object) in routes that were designed to be static.

h/t to @ovflowd for finding this.

This PR fixes this issue by simply not accessing anything that triggers static generation bailout. Unfortunately, this means we are not recording the URL for requests but we haven't yet found a way to reliably access the URL without triggering static generation bailout.

Maybe we can revisit recording the URL in the future.

@lforst lforst changed the title Add failing E2E test fix(nextjs): Don't access Request object in a way that triggers static generation bailout Dec 5, 2023
@lforst lforst changed the title fix(nextjs): Don't access Request object in a way that triggers static generation bailout fix(nextjs): Don't accidentally trigger static generation bailout Dec 5, 2023
Copy link
Contributor

github-actions bot commented Dec 5, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 65.99 KB (+0.19% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 59.59 KB (+0.22% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.12 KB (0%)
@sentry/browser - Webpack (gzipped) 21.38 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 62.97 KB (+0.41% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 29.62 KB (+0.33% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 21.7 KB (+0.46% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 198.31 KB (+0.44% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 89.73 KB (+0.47% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 64.71 KB (+0.66% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 32.29 KB (+0.22% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 66.35 KB (+0.16% 🔺)
@sentry/react - Webpack (gzipped) 21.42 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 83.07 KB (+0.14% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 48.21 KB (0%)
@sentry-internal/feedback - Webpack (gzipped) 16.17 KB (-0.13% 🔽)
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 74.63 KB (added)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 29.72 KB (added)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 71.22 KB (added)

@lforst lforst marked this pull request as ready for review December 5, 2023 13:17
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.

It's unfortunate but fine for the moment I think. It's more important we don't crash users than attaching this extra data IMO.

(side rant: This pattern of fullstack frameworks listening to object/property accesses is really problematic for us. Same thing happens in Sveltekit and Astro and we can't always work around it. I get it's probably necessary but it's a major problem for instrumentation)

@ovflowd
Copy link

ovflowd commented Dec 5, 2023

It's unfortunate but fine for the moment I think. It's more important we don't crash users than attaching this extra data IMO.

(side rant: This pattern of fullstack frameworks listening to object/property accesses is really problematic for us. Same thing happens in Sveltekit and Astro and we can't always work around it. I get it's probably necessary but it's a major problem for instrumentation)

I agree with your side rant. Maybe as a semi work around you could check if the route in question uses static generation 🤔

@lforst
Copy link
Member Author

lforst commented Dec 5, 2023

It's unfortunate but fine for the moment I think. It's more important we don't crash users than attaching this extra data IMO.
(side rant: This pattern of fullstack frameworks listening to object/property accesses is really problematic for us. Same thing happens in Sveltekit and Astro and we can't always work around it. I get it's probably necessary but it's a major problem for instrumentation)

I agree with your side rant. Maybe as a semi work around you could check if the route in question uses static generation 🤔

@ovflowd I tried that in #9729 but it looks like there is no reliable way to check that. The framework may at any point change and break this again. 😭

@lforst lforst merged commit e82be42 into develop Dec 5, 2023
56 checks passed
@lforst lforst deleted the lforst-work-around-request-proxy branch December 5, 2023 14:29
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.

None yet

4 participants