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): Exclude build-time files from page dependency manifests #6058
fix(nextjs): Exclude build-time files from page dependency manifests #6058
Conversation
a85519a
to
e932b14
Compare
size-limit report 📦
|
a1cc068
to
1c9d654
Compare
@@ -0,0 +1,34 @@ | |||
This is a [Next.js](https://nextjs.org/) project bootstrapped with [`create-next-app`](https://github.com/vercel/next.js/tree/canary/packages/create-next-app). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Should we delete this file?
@@ -5,6 +5,7 @@ import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin'; | |||
import * as chalk from 'chalk'; | |||
import * as fs from 'fs'; | |||
import * as path from 'path'; | |||
import { WebpackPluginInstance } from 'webpack'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: Since we're only using it as a type, would it make sense to do import type
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it turns out I had to run it through types.ts
because of pahen/madge#306. (See the comment I left in the file.) But in any case, I changed the import from types.ts
to be a type import.
return proto.constructor.name === 'TraceEntryPointsPlugin'; | ||
}) as WebpackPluginInstance & { excludeFiles: string[] }; | ||
if (nftPlugin) { | ||
nftPlugin.excludeFiles.push(path.join(__dirname, 'withSentryConfig.js')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: Should we be a bit more defensive here in case excludeFiles
is undefined
or whatever, or when Next.js changes stuff in future updates? Maybe we wrap this in a try-catch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it pushed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Line 115.
nftPlugin.excludeFiles = nftPlugin.excludeFiles || [];
I suppose I could also make sure it's an array. One sec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so I went with only using it if it's an array. If it's undefined, I could set it to be an array, but if it's undefined, it also means they've changed something and we should take a look. So I threw that possibility in with the branch where they've changed the data structure.
1c9d654
to
c674039
Compare
6919d55
to
aebfe0e
Compare
aebfe0e
to
280aee2
Compare
When nextjs 12+ builds a user's app, it creates a
.nft.json
file for each page, listing all of the files upon which the page depends. This is used, for example, by Vercel, when it's bundling each page into its own lambda function.Under the hood, nextjs uses the
@vercel/nft
package to do the dependency tracing. Unfortunately, it seems to run before treeshaking, so any file imported into a given module will be included, even if the thing which is imported from that file gets treeshaken away. In our case, that means that even thoughwithSentryConfig
will never be called in anyone's app (and therefore none of its dependents - not only our loaders but also sucrase and rollup, among other things - should be included in any page's dependency list), the fact thatSentry.init
is included in a user's app, and thatwithSentryConfig
is exported fromindex.server.js
right alongsideinit
, means that in fact files fromsrc/config
are included when they shouldn't be.Fortunately, nextjs runs
@vercel/nft
through a webpack plugin, and that plugin takes an option to exclude files. We therefore can addsrc/config/withSentryConfig.ts
to that option's array when we're modifying the app's webpack config and it will prevent that file (and any of its descendants) from being included.Notes:
Files in
src/config
come in three flavors: files modifying the user's webpack config, templates read in by our loaders, and files referenced by the code we inject (the wrappers). For historical reasons,src/config/index.ts
only contains the first kind of file. Since it's not actually indexing all three kinds of files, I instead renamed itwithSentryConfig.ts
, after the only thing it exports. This not only makes it clearer what it's about, it also doesn't give the false impression that it's the single export point for everything insrc/config
.To test this, I took a first stab at an integration test system focused on testing the code we use to modify the user's build. It's not especially fancy, and I'm very open to opinions on how it could be better, but it seems for the moment to at least do the trick we currently need.
One issue this PR doesn't address is the fact that we still have files from the browser SDK included in server-side page dependency manifests, because
index.server.ts
exports our react error boundary, and@sentry/react
imports from@sentry/browser
. It would take some work to make@sentry/react
depend a little less on@sentry/browser
and a little more on@sentry/utils
and@sentry/core
, such that we'd be able to exclude all files from@sentry/browser
, so that will have to happen in a separate PR.