From 049e6e7952ce34e29238ca2bff71924200654e08 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 27 Jul 2022 02:50:19 -0700 Subject: [PATCH] fix(nextjs): Move `userNextConfig.sentry` to closure (#5473) As of https://github.com/vercel/next.js/pull/38498, nextjs now warns (voluminously) if the config exported from `next.config.js` contains properties it doesn't recognize, including the `sentry` property we've been having people use to do things like [disable the webpack plugin](https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/#disable-sentrywebpackplugin). In order to prevent these warnings, the final config returned from `withSentryConfig` must not have such a property, so this deletes the property from the `userNextConfig` object before returning it. If we were to stop there, the warning would be gone, but so would the data `userNextConfig.sentry` contains. (This is true even if we run `constructWebpackConfigFunction` before doing the deletion, because the place we need the `userNextConfig.sentry` data is not in `constructWebpackConfigFunction` itself but in the function it returns. By the time that function is called by nextjs, the deletion will already have gone through and the data will be gone unless we keep a reference to it elsewhere.) Therefore, in order to allow the returned function to retain access the `userNextConfig.sentry` data, this captures it before it's deleted and passes it to `constructWebpackConfigFunction`, where it lives in a closure around the returned function. --- packages/nextjs/src/config/index.ts | 21 +++++++++++++++-- packages/nextjs/src/config/types.ts | 33 +++++++++++++++------------ packages/nextjs/src/config/webpack.ts | 17 ++++++++------ packages/nextjs/test/config.test.ts | 17 +++++++++----- 4 files changed, 58 insertions(+), 30 deletions(-) diff --git a/packages/nextjs/src/config/index.ts b/packages/nextjs/src/config/index.ts index 90bc37c4d7d1..7138dbdf0e37 100644 --- a/packages/nextjs/src/config/index.ts +++ b/packages/nextjs/src/config/index.ts @@ -17,16 +17,33 @@ export function withSentryConfig( if (typeof userNextConfig === 'function') { return function (phase: string, defaults: { defaultConfig: NextConfigObject }): Partial { const materializedUserNextConfig = userNextConfig(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 } = materializedUserNextConfig; + delete materializedUserNextConfig.sentry; + return { ...materializedUserNextConfig, - webpack: constructWebpackConfigFunction(materializedUserNextConfig, userSentryWebpackPluginOptions), + webpack: constructWebpackConfigFunction( + materializedUserNextConfig, + userSentryWebpackPluginOptions, + userSentryOptions, + ), }; }; } // Otherwise, we can just merge their config with ours and return an object. + + // 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 } = userNextConfig; + delete userNextConfig.sentry; + return { ...userNextConfig, - webpack: constructWebpackConfigFunction(userNextConfig, userSentryWebpackPluginOptions), + webpack: constructWebpackConfigFunction(userNextConfig, userSentryWebpackPluginOptions, userSentryOptions), }; } diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts index 8c0c39f1ef04..e1de856ab26b 100644 --- a/packages/nextjs/src/config/types.ts +++ b/packages/nextjs/src/config/types.ts @@ -17,26 +17,29 @@ export type NextConfigObject = { target: 'server' | 'experimental-serverless-trace'; // the output directory for the built app (defaults to ".next") distDir: string; - sentry?: { - disableServerWebpackPlugin?: boolean; - disableClientWebpackPlugin?: boolean; - hideSourceMaps?: boolean; - - // Force webpack to apply the same transpilation rules to the SDK code as apply to user code. Helpful when targeting - // older browsers which don't support ES6 (or ES6+ features like object spread). - transpileClientSDK?: boolean; - // Upload files from `/static/chunks` rather than `/static/chunks/pages`. Usually files outside of - // `pages/` only contain third-party code, but in cases where they contain user code, restricting the webpack - // plugin's upload breaks sourcemaps for those user-code-containing files, because it keeps them from being - // uploaded. At the same time, we don't want to widen the scope if we don't have to, because we're guaranteed to end - // up uploading too many files, which is why this defaults to `false`. - widenClientFileUpload?: boolean; - }; + sentry?: UserSentryOptions; } & { // other `next.config.js` options [key: string]: unknown; }; +export type UserSentryOptions = { + disableServerWebpackPlugin?: boolean; + disableClientWebpackPlugin?: boolean; + hideSourceMaps?: boolean; + + // Force webpack to apply the same transpilation rules to the SDK code as apply to user code. Helpful when targeting + // older browsers which don't support ES6 (or ES6+ features like object spread). + transpileClientSDK?: boolean; + + // Upload files from `/static/chunks` rather than `/static/chunks/pages`. Usually files outside of + // `pages/` only contain third-party code, but in cases where they contain user code, restricting the webpack + // plugin's upload breaks sourcemaps for those user-code-containing files, because it keeps them from being + // uploaded. At the same time, we don't want to widen the scope if we don't have to, because we're guaranteed to end + // up uploading too many files, which is why this defaults to `false`. + widenClientFileUpload?: boolean; +}; + export type NextConfigFunction = ( phase: string, defaults: { defaultConfig: NextConfigObject }, diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 972fbf75351c..9432bad34ed5 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -10,6 +10,7 @@ import { EntryPropertyObject, NextConfigObject, SentryWebpackPluginOptions, + UserSentryOptions, WebpackConfigFunction, WebpackConfigObject, WebpackEntryProperty, @@ -37,6 +38,7 @@ export { SentryWebpackPlugin }; export function constructWebpackConfigFunction( userNextConfig: Partial = {}, userSentryWebpackPluginOptions: Partial = {}, + userSentryOptions: UserSentryOptions = {}, ): WebpackConfigFunction { // Will be called by nextjs and passed its default webpack configuration and context data about the build (whether // we're building server or client, whether we're in dev, what version of webpack we're using, etc). Note that @@ -122,9 +124,7 @@ export function constructWebpackConfigFunction( // with the `--ignore-scripts` option, this will be blocked and the missing binary will cause an error when users // try to build their apps.) ensureCLIBinaryExists() && - (isServer - ? !userNextConfig.sentry?.disableServerWebpackPlugin - : !userNextConfig.sentry?.disableClientWebpackPlugin); + (isServer ? !userSentryOptions.disableServerWebpackPlugin : !userSentryOptions.disableClientWebpackPlugin); if (enableWebpackPlugin) { // TODO Handle possibility that user is using `SourceMapDevToolPlugin` (see @@ -138,12 +138,14 @@ export function constructWebpackConfigFunction( // the browser won't look for them and throw errors into the console when it can't find them. Because this is a // front-end-only problem, and because `sentry-cli` handles sourcemaps more reliably with the comment than // without, the option to use `hidden-source-map` only applies to the client-side build. - newConfig.devtool = userNextConfig.sentry?.hideSourceMaps && !isServer ? 'hidden-source-map' : 'source-map'; + newConfig.devtool = userSentryOptions.hideSourceMaps && !isServer ? 'hidden-source-map' : 'source-map'; } newConfig.plugins = newConfig.plugins || []; newConfig.plugins.push( - new SentryWebpackPlugin(getWebpackPluginOptions(buildContext, userSentryWebpackPluginOptions)), + new SentryWebpackPlugin( + getWebpackPluginOptions(buildContext, userSentryWebpackPluginOptions, userSentryOptions), + ), ); } @@ -381,6 +383,7 @@ function shouldAddSentryToEntryPoint(entryPointName: string, isServer: boolean): export function getWebpackPluginOptions( buildContext: BuildContext, userPluginOptions: Partial, + userSentryOptions: UserSentryOptions, ): SentryWebpackPluginOptions { const { buildId, isServer, webpack, config: userNextConfig, dev: isDev, dir: projectDir } = buildContext; const distDir = userNextConfig.distDir ?? '.next'; // `.next` is the default directory @@ -396,14 +399,14 @@ export function getWebpackPluginOptions( isWebpack5 ? [{ paths: [`${distDir}/server/chunks/`], urlPrefix: `${urlPrefix}/server/chunks` }] : [], ); - const clientInclude = userNextConfig.sentry?.widenClientFileUpload + const clientInclude = userSentryOptions.widenClientFileUpload ? [{ paths: [`${distDir}/static/chunks`], urlPrefix: `${urlPrefix}/static/chunks` }] : [{ paths: [`${distDir}/static/chunks/pages`], urlPrefix: `${urlPrefix}/static/chunks/pages` }]; const defaultPluginOptions = dropUndefinedKeys({ include: isServer ? serverInclude : clientInclude, ignore: - isServer || !userNextConfig.sentry?.widenClientFileUpload + isServer || !userSentryOptions.widenClientFileUpload ? [] : // Widening the upload scope is necessarily going to lead to us uploading files we don't need to (ones which // don't include any user code). In order to lessen that where we can, exclude the internal nextjs files we know diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index 33e8533b89ce..1ca18e191f9c 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -200,6 +200,7 @@ async function materializeFinalWebpackConfig(options: { const webpackConfigFunction = constructWebpackConfigFunction( materializedUserNextConfig, userSentryWebpackPluginConfig, + materializedUserNextConfig.sentry, ); // call it to get concrete values for comparison @@ -870,9 +871,11 @@ describe('Sentry webpack plugin config', () => { [getBuildContext('server', {}, '4'), '.next'], [getBuildContext('server', {}, '5'), '.next'], ])('`distDir` is not defined', (buildContext: BuildContext, expectedDistDir) => { - const includePaths = getWebpackPluginOptions(buildContext, { - /** userPluginOptions */ - }).include as { paths: [] }[]; + const includePaths = getWebpackPluginOptions( + buildContext, + {}, // userPluginOptions + {}, // userSentryOptions + ).include as { paths: [] }[]; for (const pathDescriptor of includePaths) { for (const path of pathDescriptor.paths) { @@ -887,9 +890,11 @@ describe('Sentry webpack plugin config', () => { [getBuildContext('server', { distDir: 'tmpDir' }, '4'), 'tmpDir'], [getBuildContext('server', { distDir: 'tmpDir' }, '5'), 'tmpDir'], ])('`distDir` is defined', (buildContext: BuildContext, expectedDistDir) => { - const includePaths = getWebpackPluginOptions(buildContext, { - /** userPluginOptions */ - }).include as { paths: [] }[]; + const includePaths = getWebpackPluginOptions( + buildContext, + {}, // userPluginOptions + {}, // userSentryOptions + ).include as { paths: [] }[]; for (const pathDescriptor of includePaths) { for (const path of pathDescriptor.paths) {