fix(nextjs): Exclude build-time files from page dependency manifests (#…

…6058)

When nextjs 12+ builds a user's app, it creates a `.nft.json` file for each page[1], 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[2] 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 though `withSentryConfig` 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 that `Sentry.init` _is_ included in a user's app, and that `withSentryConfig` is exported from `index.server.js` right alongside `init`, means that in fact files from`src/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 add `src/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 it `withSentryConfig.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 in `src/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.

[1] https://nextjs.org/docs/advanced-features/output-file-tracing
[2] https://github.com/vercel/nft

packages/nextjs/.eslintrc.js

@@ -18,5 +18,19 @@ module.exports = {
         project: ['../../tsconfig.dev.json'],
       },
     },
+    {
+      files: ['test/buildProcess/**'],
+      parserOptions: {
+        sourceType: 'module',
+      },
+      plugins: ['react'],
+      extends: ['../../.eslintrc.js', 'plugin:react/recommended'],
+      rules: {
+        // Prop types validation is not useful in test environments
+        'react/prop-types': 'off',
+        // Nextjs takes care of including react, so we don't explicitly need to
+        'react/react-in-jsx-scope': 'off',
+      },
+    },
   ],
 };

packages/nextjs/jest.config.js

@@ -1 +1,8 @@
-module.exports = require('../../jest/jest.config.js');
+const baseConfig = require('../../jest/jest.config.js');
+
+module.exports = {
+  ...baseConfig,
+  // This prevents the build tests from running when unit tests run. (If they do, they fail, because the build being
+  // tested hasn't necessarily run yet.)
+  testPathIgnorePatterns: ['<rootDir>/test/buildProcess/'],
+};

packages/nextjs/package.json

@@ -34,6 +34,7 @@
   "devDependencies": {
     "@sentry/nextjs": "7.20.1",
     "@types/webpack": "^4.41.31",
+    "eslint-plugin-react": "^7.31.11",
     "next": "10.1.3"
   },
   "peerDependencies": {
@@ -65,7 +66,8 @@
     "lint:eslint": "eslint . --cache --cache-location '../../eslintcache/' --format stylish",
     "lint:prettier": "prettier --check \"{src,test,scripts}/**/*.ts\"",
     "test": "run-s test:unit",
-    "test:all": "run-s test:unit test:integration",
+    "test:all": "run-s test:unit test:integration test:build",
+    "test:build": "yarn ts-node test/buildProcess/runTest.ts",
     "test:unit": "jest",
     "test:integration": "test/run-integration-tests.sh && yarn test:types",
     "test:types": "cd test/types && yarn test",

packages/nextjs/src/config/types.ts

@@ -3,6 +3,11 @@ import { WebpackPluginInstance } from 'webpack';
 
 export type SentryWebpackPluginOptions = SentryCliPluginOptions;
 export type SentryWebpackPlugin = WebpackPluginInstance & { options: SentryWebpackPluginOptions };
+// Export this from here because importing something from Webpack (the library) in `webpack.ts` confuses the heck out of
+// madge, which we use for circular dependency checking. We've manually excluded this file from the check (which is
+// safe, since it only includes types), so we can import it here without causing madge to fail. See
+// https://github.com/pahen/madge/issues/306.
+export type { WebpackPluginInstance };
 
 /**
  * Overall Nextjs config

packages/nextjs/src/config/webpack.ts

@@ -6,7 +6,9 @@ import * as chalk from 'chalk';
 import * as fs from 'fs';
 import * as path from 'path';
 
-import {
+// Note: If you need to import a type from Webpack, do it in `types.ts` and export it from there. Otherwise, our
+// circular dependency check thinks this file is importing from itself. See https://github.com/pahen/madge/issues/306.
+import type {
   BuildContext,
   EntryPropertyObject,
   NextConfigObject,
@@ -16,6 +18,7 @@ import {
   WebpackConfigObject,
   WebpackEntryProperty,
   WebpackModuleRule,
+  WebpackPluginInstance,
 } from './types';
 
 export { SentryWebpackPlugin };
@@ -100,6 +103,30 @@ export function constructWebpackConfigFunction(
           ],
         });
       }
+
+      // Prevent `@vercel/nft` (which nextjs uses to determine which files are needed when packaging up a lambda) from
+      // including any of our build-time code or dependencies. (Otherwise it'll include files like this one and even the
+      // entirety of rollup and sucrase.)
+      const nftPlugin = newConfig.plugins?.find((plugin: WebpackPluginInstance) => {
+        const proto = Object.getPrototypeOf(plugin) as WebpackPluginInstance;
+        return proto.constructor.name === 'TraceEntryPointsPlugin';
+      }) as WebpackPluginInstance & { excludeFiles: string[] };
+      if (nftPlugin) {
+        if (Array.isArray(nftPlugin.excludeFiles)) {
+          nftPlugin.excludeFiles.push(path.join(__dirname, 'withSentryConfig.js'));
+        } else {
+          __DEBUG_BUILD__ &&
+            logger.warn(
+              'Unable to exclude Sentry build-time helpers from nft files. `TraceEntryPointsPlugin.excludeFiles` is not ' +
+                'an array. This is a bug; please report this to Sentry:  https://github.com/getsentry/sentry-javascript/issues/.',
+            );
+        }
+      } else {
+        __DEBUG_BUILD__ &&
+          logger.warn(
+            'Unable to exclude Sentry build-time helpers from nft files. Could not find `TraceEntryPointsPlugin`.',
+          );
+      }
     }
 
     // The SDK uses syntax (ES6 and ES6+ features like object spread) which isn't supported by older browsers. For users

packages/nextjs/src/index.server.ts

@@ -145,7 +145,7 @@ const deprecatedIsBuild = (): boolean => isBuild();
 export { deprecatedIsBuild as isBuild };
 
 export type { SentryWebpackPluginOptions } from './config/types';
-export { withSentryConfig } from './config';
+export { withSentryConfig } from './config/withSentryConfig';
 export {
   withSentryGetServerSideProps,
   withSentryGetStaticProps,

packages/nextjs/test/buildProcess/jest.config.js

@@ -0,0 +1,4 @@
+// In order to not have the build tests run as part of the unit test suite, we exclude them in
+// `packages/nextjs/jest.config.js`. The resets the test-matching regex so that when `runTest.ts` calls `yarn jest` with
+// ths config file, jest will find the tests in `tests`.
+module.exports = require('../../../../jest/jest.config.js');

packages/nextjs/test/buildProcess/runTest.ts

@@ -0,0 +1,42 @@
+/* eslint-disable no-console */
+import * as childProcess from 'child_process';
+import * as path from 'path';
+import { sync as rimrafSync } from 'rimraf';
+
+const TEST_APP_DIR = 'test/buildProcess/testApp';
+
+/**
+ * Run the given shell command, piping the shell process's `stdin`, `stdout`, and `stderr` to that of the current
+ * process if this script is run with the `--debug` flag. Returns contents of `stdout`.
+ */
+function run(cmd: string, options?: childProcess.ExecSyncOptions): string {
+  return String(
+    childProcess.execSync(cmd, {
+      stdio: process.argv.includes('--debug') ? 'inherit' : 'ignore',
+      ...options,
+    }),
+  );
+}
+
+// Note: We use a file dependency for the SDK, rather than linking it, because if it's linked, nextjs rolls the entire
+// SDK and all of its dependencies into a bundle, making it impossible to tell (from the NFT file, at least) what's
+// being included.
+console.log('Installing dependencies...');
+process.chdir(TEST_APP_DIR);
+rimrafSync('node_modules');
+run('yarn');
+
+console.log('Building app...');
+rimrafSync('.next');
+run('yarn build');
+
+console.log('App built. Running tests...');
+process.chdir('..');
+const jestConfigFile = path.resolve(process.cwd(), 'jest.config.js');
+try {
+  // We have to specify the config file explicitly because otherwise it'll use the one at the root level of
+  // `packages/nextjs`, since that's where the closest `package.json` file is
+  run(`yarn jest --config ${jestConfigFile} tests`, { stdio: 'inherit' });
+} catch (err) {
+  console.log('\nNot all build process tests passed.');
+}

packages/nextjs/test/buildProcess/testApp/.gitignore

@@ -0,0 +1,36 @@
+# See https://help.github.com/articles/ignoring-files/ for more about ignoring files.
+
+# dependencies
+/node_modules
+/.pnp
+.pnp.js
+
+# testing
+/coverage
+
+# next.js
+/.next/
+/out/
+
+# production
+/build
+
+# misc
+.DS_Store
+*.pem
+
+# debug
+npm-debug.log*
+yarn-debug.log*
+yarn-error.log*
+.pnpm-debug.log*
+
+# local env files
+.env*.local
+
+# vercel
+.vercel
+
+# typescript
+*.tsbuildinfo
+next-env.d.ts

packages/nextjs/test/buildProcess/testApp/next.config.js

@@ -0,0 +1,25 @@
+// const nextConfig = {
+// }
+//
+// module.exports = nextConfig
+
+const { withSentryConfig } = require('@sentry/nextjs');
+
+const moduleExports = {
+  reactStrictMode: true,
+  swcMinify: true,
+  eslint: {
+    ignoreDuringBuilds: true,
+  },
+  sentry: {
+    // Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts`
+    // TODO (v8): This can come out in v8, because this option will get a default value
+    hideSourceMaps: false,
+  },
+};
+const SentryWebpackPluginOptions = {
+  dryRun: true,
+  silent: true,
+};
+
+module.exports = withSentryConfig(moduleExports, SentryWebpackPluginOptions);

packages/nextjs/test/buildProcess/testApp/package.json

@@ -0,0 +1,18 @@
+{
+  "name": "testapp",
+  "version": "0.1.0",
+  "private": true,
+  "scripts": {
+    "dev": "next dev",
+    "build": "next build",
+    "start": "next start",
+    "lint": "next lint"
+  },
+  "dependencies": {
+    "@sentry/nextjs": "file:../../../",
+    "@vercel/nft": "latest",
+    "next": "13.0.1",
+    "react": "18.2.0",
+    "react-dom": "18.2.0"
+  }
+}

packages/nextjs/test/buildProcess/testApp/sentry.client.config.js

@@ -0,0 +1,5 @@
+import * as Sentry from '@sentry/nextjs';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+});

packages/nextjs/test/buildProcess/testApp/sentry.server.config.js

@@ -0,0 +1,5 @@
+import * as Sentry from '@sentry/nextjs';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+});

packages/nextjs/test/buildProcess/testApp/src/dogs.json

@@ -0,0 +1,3 @@
+{
+  "dogs": "are great"
+}

packages/nextjs/test/buildProcess/testApp/src/pages/_app.js

@@ -0,0 +1,5 @@
+function MyApp({ Component, pageProps }) {
+  return <Component {...pageProps} />;
+}
+
+export default MyApp;

packages/nextjs/test/buildProcess/testApp/src/pages/api/dogs.js

@@ -0,0 +1,8 @@
+import * as fs from 'fs';
+import * as path from 'path';
+
+export default function handler(req, res) {
+  const dogData = JSON.parse(fs.readFileSync(path.resolve('../../dogs.json')));
+  dogData.test = 'something';
+  res.status(200).json(dogData);
+}

packages/nextjs/test/buildProcess/testApp/src/pages/index.js

@@ -0,0 +1,3 @@
+export default function Home() {
+  return <div>This is the homepage.</div>;
+}