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) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 36368f287aab..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'; @@ -27,6 +34,14 @@ 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. +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. * @@ -106,14 +121,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( diff --git a/packages/nextjs/src/config/withSentryConfig.ts b/packages/nextjs/src/config/withSentryConfig.ts index 43da68e3df6b..4470875f4d8e 100644 --- a/packages/nextjs/src/config/withSentryConfig.ts +++ b/packages/nextjs/src/config/withSentryConfig.ts @@ -1,5 +1,11 @@ -import type { ExportedNextConfig, NextConfigFunction, NextConfigObject, SentryWebpackPluginOptions } from './types'; -import { constructWebpackConfigFunction } from './webpack'; +import { isBuild } from '../utils/isBuild'; +import type { + ExportedNextConfig, + NextConfigFunction, + NextConfigObject, + NextConfigObjectWithSentry, + SentryWebpackPluginOptions, +} from './types'; /** * Add Sentry options to the config to be exported from the user's `next.config.js` file. @@ -16,38 +22,42 @@ 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; + + // 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), + }; + } - return { - ...userNextConfigObject, - webpack: constructWebpackConfigFunction(userNextConfigObject, userSentryWebpackPluginOptions, userSentryOptions), - }; + // At runtime, we just return the user's config untouched. + return userNextConfigObject; } 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/index.test.ts b/packages/nextjs/test/config/withSentryConfig.test.ts similarity index 57% rename from packages/nextjs/test/config/index.test.ts rename to packages/nextjs/test/config/withSentryConfig.test.ts index 7ae6589c8231..625cc964710d 100644 --- a/packages/nextjs/test/config/index.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 (global as any)._sentryWebpackModuleLoaded; + + materializeFinalNextConfig(exportedNextConfig); + + expect((global as any)._sentryWebpackModuleLoaded).toBe(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 (global as any)._sentryWebpackModuleLoaded; + + materializeFinalNextConfig(exportedNextConfig); + + expect((global as any)._sentryWebpackModuleLoaded).toBeUndefined(); + }); + }); + }); });