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

ref(nextjs): Use bundling instead of proxying to wrap pages and API routes #6685

Merged
merged 12 commits into from
Jan 10, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Jan 8, 2023

Ref #6120

To automatically wrap data fetchers and API routes in the Next.js SDK we are currently using an approach we call "proxying". What this means is that we inject a webpack loader that replaces the page/API route files with a "proxy file" that imports the original page/API route file, wraps all data fetchers and API routes, and reexports everything.

In order to distinguish the original file from our proxy file within the loader, we're importing/exporting the user file with a query string at the end. It looks like this: export * from '{user-module}?sentry-wrapped'. Unfortunately, this query string messes up a bunch of things:

  1. It breaks @vercel/nft (Node File Trace) which Vercel uses during the Next.js build process to determine which files are needed (issue)
  2. It breaks source maps with Webpack 4 or when the experimental-serverless-trace target is set in the Next.js config (issue)

This PR fixes the issues above by replacing our "proxying" approach with a slightly different one. We will still be using a loader but instead of letting webpack take care of the "proxying" we simply return a bundled version of our wrapper code and the user code. For this bundling we are using rollup.

Fixes #6118
Fixes #5998

@mydea
Copy link
Member

mydea commented Jan 9, 2023

Generally, I think this is a great approach, and probably more robust than what we had before.

I am really not an expert in this part of the code at all, so mainly I have two questions, to be sure:

  1. You mention it in comments, but source maps stay correct with this? May users be confused by e.g. line numbers being shifted or similar?
  2. Is there a noticeable impact on build time? I know this is hard to measure, but just a thought to keep this in mind.

@lforst lforst marked this pull request as ready for review January 9, 2023 09:54
@lforst lforst requested review from mydea and Lms24 January 9, 2023 09:57
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.85 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 61.46 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.62 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 55 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.38 KB (0%)
@sentry/browser - Webpack (minified) 66.55 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.4 KB (-0.01% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.63 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.82 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.25 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.06 KB (0%)
@sentry/replay - Webpack (gzipped + minified) 38.3 KB (0%)

@lforst
Copy link
Member Author

lforst commented Jan 9, 2023

  1. You mention it in comments, but source maps stay correct with this? May users be confused by e.g. line numbers being shifted or similar?

Source maps stay correct! Or rather they will become correct when using webpack 4. I also took a look at the fingerprinting and it looks the same.

  1. Is there a noticeable impact on build time? I know this is hard to measure, but just a thought to keep this in mind.

No. The build time impact is negligible. For each user file, we're only bundling 2 files, our wrapper module, and the user module. We're not traversing the entire dependency tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants