From 4b40f61d62bbffc3faeb6764876d3959ada3538f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 23 Nov 2022 00:02:56 -0800 Subject: [PATCH 1/8] extract main logic in `withSentryConfig` into a helper function --- .../nextjs/src/config/withSentryConfig.ts | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/packages/nextjs/src/config/withSentryConfig.ts b/packages/nextjs/src/config/withSentryConfig.ts index 43da68e3df6b..4e42550587cd 100644 --- a/packages/nextjs/src/config/withSentryConfig.ts +++ b/packages/nextjs/src/config/withSentryConfig.ts @@ -1,4 +1,10 @@ -import type { ExportedNextConfig, NextConfigFunction, NextConfigObject, SentryWebpackPluginOptions } from './types'; +import type { + ExportedNextConfig, + NextConfigFunction, + NextConfigObject, + NextConfigObjectWithSentry, + SentryWebpackPluginOptions, +} from './types'; import { constructWebpackConfigFunction } from './webpack'; /** @@ -16,35 +22,29 @@ export function withSentryConfig( // `defaults` in order to pass them along to the user's function if (typeof exportedUserNextConfig === 'function') { return function (phase: string, defaults: { defaultConfig: NextConfigObject }): NextConfigObject { - let userNextConfigObject = exportedUserNextConfig(phase, defaults); + const userNextConfigObject = exportedUserNextConfig(phase, defaults); - // Next 12.2.3+ warns about non-canonical properties on `userNextConfig`, so grab and then remove the `sentry` - // property there. Where we actually need it is in the webpack config function we're going to create, so pass it - // to `constructWebpackConfigFunction` so that it will be in the created function's closure. - const { sentry: userSentryOptions } = userNextConfigObject; - delete userNextConfigObject.sentry; - // Remind TS that there's now no `sentry` property - userNextConfigObject = userNextConfigObject as NextConfigObject; - - return { - ...userNextConfigObject, - webpack: constructWebpackConfigFunction( - userNextConfigObject, - userSentryWebpackPluginOptions, - userSentryOptions, - ), - }; + return getFinalConfigObject(userNextConfigObject, userSentryWebpackPluginOptions); }; } // Otherwise, we can just merge their config with ours and return an object. + return getFinalConfigObject(exportedUserNextConfig, userSentryWebpackPluginOptions); +} - // Prevent nextjs from getting mad about having a non-standard config property in `userNextConfig`. (See note above - // for a more thorough explanation of what we're doing here.) - const { sentry: userSentryOptions } = exportedUserNextConfig; - delete exportedUserNextConfig.sentry; +// Modify the materialized object form of the user's next config by deleting the `sentry` property and wrapping the +// `webpack` property +function getFinalConfigObject( + incomingUserNextConfigObject: NextConfigObjectWithSentry, + userSentryWebpackPluginOptions: Partial, +): NextConfigObject { + // Next 12.2.3+ warns about non-canonical properties on `userNextConfig`, so grab and then remove the `sentry` + // property there. Where we actually need it is in the webpack config function we're going to create, so pass it + // to `constructWebpackConfigFunction` so that it can live in the returned function's closure. + const { sentry: userSentryOptions } = incomingUserNextConfigObject; + delete incomingUserNextConfigObject.sentry; // Remind TS that there's now no `sentry` property - const userNextConfigObject = exportedUserNextConfig as NextConfigObject; + const userNextConfigObject = incomingUserNextConfigObject as NextConfigObject; return { ...userNextConfigObject, From 650cd953cfab70101bbde27952d908475dd24c61 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 23 Nov 2022 00:04:40 -0800 Subject: [PATCH 2/8] only require `webpack.ts` at build time --- .../nextjs/src/config/withSentryConfig.ts | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/nextjs/src/config/withSentryConfig.ts b/packages/nextjs/src/config/withSentryConfig.ts index 4e42550587cd..4470875f4d8e 100644 --- a/packages/nextjs/src/config/withSentryConfig.ts +++ b/packages/nextjs/src/config/withSentryConfig.ts @@ -1,3 +1,4 @@ +import { isBuild } from '../utils/isBuild'; import type { ExportedNextConfig, NextConfigFunction, @@ -5,7 +6,6 @@ import type { NextConfigObjectWithSentry, SentryWebpackPluginOptions, } from './types'; -import { constructWebpackConfigFunction } from './webpack'; /** * Add Sentry options to the config to be exported from the user's `next.config.js` file. @@ -46,8 +46,18 @@ function getFinalConfigObject( // Remind TS that there's now no `sentry` property const userNextConfigObject = incomingUserNextConfigObject as NextConfigObject; - return { - ...userNextConfigObject, - webpack: constructWebpackConfigFunction(userNextConfigObject, userSentryWebpackPluginOptions, userSentryOptions), - }; + // In order to prevent all of our build-time code from being bundled in people's route-handling serverless functions, + // we exclude `webpack.ts` and all of its dependencies from nextjs's `@vercel/nft` filetracing. We therefore need to + // make sure that we only require it at build time. + if (isBuild()) { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { constructWebpackConfigFunction } = require('./webpack'); + return { + ...userNextConfigObject, + webpack: constructWebpackConfigFunction(userNextConfigObject, userSentryWebpackPluginOptions, userSentryOptions), + }; + } + + // At runtime, we just return the user's config untouched. + return userNextConfigObject; } From 7e1b8bcf466e7dbd5f5f2bf8594ed123c8dd6014 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 23 Nov 2022 00:53:24 -0800 Subject: [PATCH 3/8] add `webpack.ts` to rollup config --- packages/nextjs/rollup.npm.config.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index dcda038ba311..07a8594a5b1f 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -5,7 +5,12 @@ export default [ makeBaseNPMConfig({ // We need to include `instrumentServer.ts` separately because it's only conditionally required, and so rollup // doesn't automatically include it when calculating the module dependency tree. - entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/utils/instrumentServer.ts'], + entrypoints: [ + 'src/index.server.ts', + 'src/index.client.ts', + 'src/utils/instrumentServer.ts', + 'src/config/webpack.ts', + ], // prevent this internal nextjs code from ending up in our built package (this doesn't happen automatially because // the name doesn't match an SDK dependency) From 5f9a08aade562797077a259b6f596f5ec1edf5ca Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 23 Nov 2022 00:05:12 -0800 Subject: [PATCH 4/8] exclude `webpack.ts` but not `withSentryConfig.ts` --- packages/nextjs/src/config/webpack.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 36368f287aab..e465f9b45e6e 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -106,14 +106,15 @@ 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.) + // entirety of rollup and sucrase.) Since this file is the root of that dependency tree, it's enough to just + // exclude it and the rest will be excluded as well. 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')); + nftPlugin.excludeFiles.push(__filename); } else { __DEBUG_BUILD__ && logger.warn( From eda94f7096c0b9fd67e6218f1055d86cb52763a7 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 23 Nov 2022 00:05:50 -0800 Subject: [PATCH 5/8] add flag noting that `webpack.ts` has been loaded --- packages/nextjs/src/config/webpack.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index e465f9b45e6e..4fc3dd7bfeee 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -27,6 +27,13 @@ export { SentryWebpackPlugin }; // TODO: merge default SentryWebpackPlugin include with their SentryWebpackPlugin include // TODO: drop merged keys from override check? `includeDefaults` option? +// In order to make sure that build-time code isn't getting bundled in runtime bundles (specifically, in the serverless +// functions into which nextjs converts a user's routes), we exclude this module (and all of its dependencies) from the +// nft file manifests nextjs generates. (See the code dealing with the `TraceEntryPointsPlugin` below.) So that we don't +// crash people, we therefore need to make sure nothing tries to either require this file or import from it. Setting +// this env variable allows us to test whether or not that's happened. +process.env.SENTRY_WEBPACK_MODULE_LOADED = 'true'; + /** * Construct the function which will be used as the nextjs config's `webpack` value. * From bf475705af0c6659c43f4f2118d6af965298d0f0 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 23 Nov 2022 00:15:09 -0800 Subject: [PATCH 6/8] rename `index.test.ts` to `withSentryConfig.test.ts` --- .../test/config/{index.test.ts => withSentryConfig.test.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename packages/nextjs/test/config/{index.test.ts => withSentryConfig.test.ts} (100%) diff --git a/packages/nextjs/test/config/index.test.ts b/packages/nextjs/test/config/withSentryConfig.test.ts similarity index 100% rename from packages/nextjs/test/config/index.test.ts rename to packages/nextjs/test/config/withSentryConfig.test.ts From a9af4aa20631a07e713809e81f92a128657c769b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 23 Nov 2022 00:53:04 -0800 Subject: [PATCH 7/8] add and fix tests --- .../test/buildProcess/tests/nft.test.ts | 9 +++-- .../test/config/withSentryConfig.test.ts | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/test/buildProcess/tests/nft.test.ts b/packages/nextjs/test/buildProcess/tests/nft.test.ts index 0915489a88b1..fb208a9cae0c 100644 --- a/packages/nextjs/test/buildProcess/tests/nft.test.ts +++ b/packages/nextjs/test/buildProcess/tests/nft.test.ts @@ -11,12 +11,15 @@ it('excludes build-time SDK dependencies from nft files', () => { // These are all of the files which control the way we modify the user's app build by changing the webpack config. // They get mistakenly included by the nft plugin because we export `withSentryConfig` from `index.server.ts`. Because - // the wrappers are referenced from the code we inject at build time, they need to stay. + // the wrappers are referenced from the code we inject at build time, they need to stay. `withSentryConfig.ts` itself + // also needs to stay because nextjs loads `next.config.js` at runtime const sentryConfigDirEntries = dogNFTjson.files.filter(entry => entry.includes('node_modules/@sentry/nextjs/build/cjs/config'), ); - const withSentryConfigEntries = sentryConfigDirEntries.filter(entry => !entry.includes('config/wrappers')); const sentryWrapperEntries = sentryConfigDirEntries.filter(entry => entry.includes('config/wrappers')); + const excludedSentryConfigEntries = sentryConfigDirEntries.filter( + entry => !sentryWrapperEntries.includes(entry) && !entry.includes('withSentryConfig'), + ); // Sucrase and rollup are dependencies of one of the webpack loaders we add to the config - also not needed at runtime. const sucraseEntries = dogNFTjson.files.filter(entry => entry.includes('node_modules/sucrase')); @@ -25,7 +28,7 @@ it('excludes build-time SDK dependencies from nft files', () => { ); // None of the build-time dependencies should be listed - expect(withSentryConfigEntries.length).toEqual(0); + expect(excludedSentryConfigEntries.length).toEqual(0); expect(sucraseEntries.length).toEqual(0); expect(rollupEntries.length).toEqual(0); diff --git a/packages/nextjs/test/config/withSentryConfig.test.ts b/packages/nextjs/test/config/withSentryConfig.test.ts index 7ae6589c8231..e726df33340c 100644 --- a/packages/nextjs/test/config/withSentryConfig.test.ts +++ b/packages/nextjs/test/config/withSentryConfig.test.ts @@ -1,6 +1,9 @@ +import * as isBuildModule from '../../src/utils/isBuild'; import { defaultsObject, exportedNextConfig, runtimePhase, userNextConfig } from './fixtures'; import { materializeFinalNextConfig } from './testUtils'; +const isBuildSpy = jest.spyOn(isBuildModule, 'isBuild').mockReturnValue(true); + describe('withSentryConfig', () => { it('includes expected properties', () => { const finalConfig = materializeFinalNextConfig(exportedNextConfig); @@ -59,4 +62,37 @@ describe('withSentryConfig', () => { // directly expect('sentry' in finalConfig).toBe(false); }); + + describe('conditional use of `constructWebpackConfigFunction`', () => { + // Note: In these tests, it would be nice to be able to spy on `constructWebpackConfigFunction` to see whether or + // not it's called, but that sets up a catch-22: If you import or require the module to spy on the function, it gets + // cached and the `require` call we care about (inside of `withSentryConfig`) doesn't actually run the module code. + // Alternatively, if we call `jest.resetModules()` after setting up the spy, then the module code *is* run a second + // time, but the spy belongs to the first instance of the module and therefore never registers a call. Thus we have + // to test whether or not the file is required instead. + + it('imports from `webpack.ts` if `isBuild` returns true', () => { + jest.isolateModules(() => { + // In case this is still set from elsewhere, reset it + delete process.env.SENTRY_WEBPACK_MODULE_LOADED; + + materializeFinalNextConfig(exportedNextConfig); + + expect(process.env.SENTRY_WEBPACK_MODULE_LOADED).toEqual('true'); + }); + }); + + it("doesn't import from `webpack.ts` if `isBuild` returns false", () => { + jest.isolateModules(() => { + isBuildSpy.mockReturnValueOnce(false); + + // In case this is still set from elsewhere, reset it + delete process.env.SENTRY_WEBPACK_MODULE_LOADED; + + materializeFinalNextConfig(exportedNextConfig); + + expect(process.env.SENTRY_WEBPACK_MODULE_LOADED).toBeUndefined(); + }); + }); + }); }); From 728562b0f95e88c3ea2bdc67fd124244ec1ba5cf Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 23 Nov 2022 10:28:59 +0000 Subject: [PATCH 8/8] Use global instead of `process.env` --- packages/nextjs/src/config/webpack.ts | 12 ++++++++++-- packages/nextjs/test/config/withSentryConfig.test.ts | 8 ++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 4fc3dd7bfeee..7e01b63ce85d 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -1,6 +1,13 @@ /* eslint-disable max-lines */ import { getSentryRelease } from '@sentry/node'; -import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils'; +import { + arrayify, + dropUndefinedKeys, + escapeStringForRegex, + GLOBAL_OBJ, + logger, + stringMatchesSomePattern, +} from '@sentry/utils'; import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin'; import * as chalk from 'chalk'; import * as fs from 'fs'; @@ -32,7 +39,8 @@ export { SentryWebpackPlugin }; // nft file manifests nextjs generates. (See the code dealing with the `TraceEntryPointsPlugin` below.) So that we don't // crash people, we therefore need to make sure nothing tries to either require this file or import from it. Setting // this env variable allows us to test whether or not that's happened. -process.env.SENTRY_WEBPACK_MODULE_LOADED = 'true'; +const _global = GLOBAL_OBJ as typeof GLOBAL_OBJ & { _sentryWebpackModuleLoaded?: true }; +_global._sentryWebpackModuleLoaded = true; /** * Construct the function which will be used as the nextjs config's `webpack` value. diff --git a/packages/nextjs/test/config/withSentryConfig.test.ts b/packages/nextjs/test/config/withSentryConfig.test.ts index e726df33340c..625cc964710d 100644 --- a/packages/nextjs/test/config/withSentryConfig.test.ts +++ b/packages/nextjs/test/config/withSentryConfig.test.ts @@ -74,11 +74,11 @@ describe('withSentryConfig', () => { it('imports from `webpack.ts` if `isBuild` returns true', () => { jest.isolateModules(() => { // In case this is still set from elsewhere, reset it - delete process.env.SENTRY_WEBPACK_MODULE_LOADED; + delete (global as any)._sentryWebpackModuleLoaded; materializeFinalNextConfig(exportedNextConfig); - expect(process.env.SENTRY_WEBPACK_MODULE_LOADED).toEqual('true'); + expect((global as any)._sentryWebpackModuleLoaded).toBe(true); }); }); @@ -87,11 +87,11 @@ describe('withSentryConfig', () => { isBuildSpy.mockReturnValueOnce(false); // In case this is still set from elsewhere, reset it - delete process.env.SENTRY_WEBPACK_MODULE_LOADED; + delete (global as any)._sentryWebpackModuleLoaded; materializeFinalNextConfig(exportedNextConfig); - expect(process.env.SENTRY_WEBPACK_MODULE_LOADED).toBeUndefined(); + expect((global as any)._sentryWebpackModuleLoaded).toBeUndefined(); }); }); });