Uh oh!
There was an error while loading. Please reload this page.
fix(nextjs): Exclude build-time files from page dependency manifests - #6058
Conversation
a85519a to
e932b14Comparesize-limit report 📦
|
a1cc068 to
1c9d654Compare| @@ -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). | |||
| 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.
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.
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.
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.
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.
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
c674039CompareUh oh!
There was an error while loading. Please reload this page.
6919d55 to
aebfe0eCompareaebfe0e to
280aee2Compare
When nextjs 12+ builds a user's app, it creates a
.nft.jsonfile 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/nftpackage 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 thoughwithSentryConfigwill 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.initis included in a user's app, and thatwithSentryConfigis exported fromindex.server.jsright alongsideinit, means that in fact files fromsrc/configare included when they shouldn't be.Fortunately, nextjs runs
@vercel/nftthrough a webpack plugin, and that plugin takes an option to exclude files. We therefore can addsrc/config/withSentryConfig.tsto 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/configcome 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.tsonly 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.tsexports our react error boundary, and@sentry/reactimports from@sentry/browser. It would take some work to make@sentry/reactdepend a little less on@sentry/browserand a little more on@sentry/utilsand@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.