From 08d9a623f8cf42cb59ce89a2111ac4c4dd8c6011 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Jun 2021 09:32:37 -0700 Subject: [PATCH 1/8] s/pass/TODO --- packages/nextjs/test/config.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index 7023dc9ba4d3..7d6d2a134505 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -244,15 +244,15 @@ describe('webpack config', () => { describe('Sentry webpack plugin config', () => { it('includes expected properties', () => { - // pass + // TODO }); it('preserves unrelated plugin config options', () => { - // pass + // TODO }); it('warns when overriding certain default values', () => { - // pass + // TODO }); it("merges default include and ignore/ignoreFile options with user's values", () => { From f1de2aa42f1540d3699c229af530c80aa277e60d Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Jun 2021 09:34:48 -0700 Subject: [PATCH 2/8] split out server and client build contexts --- packages/nextjs/test/config.test.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index 7d6d2a134505..cd9b71c2985e 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -47,7 +47,8 @@ const clientWebpackConfig = { target: 'web', context: '/Users/Maisey/projects/squirrelChasingSimulator', }; -const buildContext = { isServer: true, dev: false, buildId: 'doGsaREgReaT' }; +const serverBuildContext = { isServer: true, dev: false, buildId: 'doGsaREgReaT' }; +const clientBuildContext = { isServer: false, dev: false, buildId: 'doGsaREgReaT' }; /** * Derive the final values of all next config options, by first applying `withSentryConfig` and then, if it returns a @@ -162,7 +163,7 @@ describe('webpack config', () => { userNextConfig, userSentryWebpackPluginConfig, incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: buildContext, + incomingWebpackBuildContext: serverBuildContext, }); expect(finalWebpackConfig).toEqual( @@ -179,12 +180,12 @@ describe('webpack config', () => { userNextConfig, userSentryWebpackPluginConfig, incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: buildContext, + incomingWebpackBuildContext: serverBuildContext, }); // Run the user's webpack config function, so we can check the results against ours. Delete `entry` because we'll // test it separately, and besides, it's one that we *should* be overwriting. - const materializedUserWebpackConfig = userNextConfig.webpack(serverWebpackConfig, buildContext); + const materializedUserWebpackConfig = userNextConfig.webpack(serverWebpackConfig, serverBuildContext); // @ts-ignore `entry` may be required in real life, but we don't need it for our tests delete materializedUserWebpackConfig.entry; @@ -197,7 +198,7 @@ describe('webpack config', () => { userNextConfig, userSentryWebpackPluginConfig, incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: buildContext, + incomingWebpackBuildContext: serverBuildContext, }); expect(finalWebpackConfig.entry).toEqual( @@ -212,7 +213,7 @@ describe('webpack config', () => { userNextConfig, userSentryWebpackPluginConfig, incomingWebpackConfig: clientWebpackConfig, - incomingWebpackBuildContext: { ...buildContext, isServer: false }, + incomingWebpackBuildContext: clientBuildContext, }); expect(finalWebpackConfig.entry).toEqual( @@ -229,7 +230,7 @@ describe('webpack config', () => { ...clientWebpackConfig, entry: () => Promise.resolve({ main: './src/index.ts', 'main.js': ['sitLieDownRollOver.config.js'] }), }, - incomingWebpackBuildContext: { ...buildContext, isServer: false }, + incomingWebpackBuildContext: clientBuildContext, }); expect(finalWebpackConfig.entry).toEqual( From e55c2d12bf13a5d13621992a247bccc12a8ce3d6 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Jun 2021 09:38:45 -0700 Subject: [PATCH 3/8] simplify mock --- packages/nextjs/test/config.test.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index cd9b71c2985e..247869e8ab89 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -7,7 +7,7 @@ import { SentryWebpackPluginOptions, WebpackConfigObject, } from '../src/config/types'; -import { SENTRY_SERVER_CONFIG_FILE, SERVER_SDK_INIT_PATH, storeServerConfigFileLocation } from '../src/config/utils'; +import { SENTRY_SERVER_CONFIG_FILE, SERVER_SDK_INIT_PATH } from '../src/config/utils'; import { constructWebpackConfigFunction, SentryWebpackPlugin } from '../src/config/webpack'; // mock `storeServerConfigFileLocation` in order to make it a no-op when necessary @@ -15,7 +15,8 @@ jest.mock('../src/config/utils', () => { const original = jest.requireActual('../src/config/utils'); return { ...original, - storeServerConfigFileLocation: jest.fn().mockImplementation(original.setRuntimeEnvVars), + // nuke this so it won't try to look for our dummy paths + storeServerConfigFileLocation: jest.fn(), }; }); @@ -153,11 +154,6 @@ describe('withSentryConfig', () => { }); describe('webpack config', () => { - beforeEach(() => { - // nuke this so it won't try to look for our dummy paths - (storeServerConfigFileLocation as jest.Mock).mockImplementationOnce(() => undefined); - }); - it('includes expected properties', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, From b942dfde738f934c22a306a9809f5d4a34fb8a54 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Jun 2021 09:42:48 -0700 Subject: [PATCH 4/8] only pass plugin config when it's relevant --- packages/nextjs/test/config.test.ts | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index 247869e8ab89..1b436b1820f6 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -63,7 +63,7 @@ const clientBuildContext = { isServer: false, dev: false, buildId: 'doGsaREgReaT */ function materializeFinalNextConfig( userNextConfig: ExportedNextConfig, - userSentryWebpackPluginConfig: SentryWebpackPluginOptions, + userSentryWebpackPluginConfig?: SentryWebpackPluginOptions, ): NextConfigObject { const sentrifiedConfig = withSentryConfig(userNextConfig, userSentryWebpackPluginConfig); let finalConfigValues = sentrifiedConfig; @@ -94,7 +94,7 @@ function materializeFinalNextConfig( */ async function materializeFinalWebpackConfig(options: { userNextConfig: ExportedNextConfig; - userSentryWebpackPluginConfig: SentryWebpackPluginOptions; + userSentryWebpackPluginConfig?: SentryWebpackPluginOptions; incomingWebpackConfig: WebpackConfigObject; incomingWebpackBuildContext: BuildContext; }): Promise { @@ -113,7 +113,7 @@ async function materializeFinalWebpackConfig(options: { describe('withSentryConfig', () => { it('includes expected properties', () => { - const finalConfig = materializeFinalNextConfig(userNextConfig, userSentryWebpackPluginConfig); + const finalConfig = materializeFinalNextConfig(userNextConfig); expect(finalConfig).toEqual( expect.objectContaining({ @@ -123,13 +123,13 @@ describe('withSentryConfig', () => { }); it('preserves unrelated next config options', () => { - const finalConfig = materializeFinalNextConfig(userNextConfig, userSentryWebpackPluginConfig); + const finalConfig = materializeFinalNextConfig(userNextConfig); expect(finalConfig.publicRuntimeConfig).toEqual(userNextConfig.publicRuntimeConfig); }); it("works when user's overall config is an object", () => { - const finalConfig = materializeFinalNextConfig(userNextConfig, userSentryWebpackPluginConfig); + const finalConfig = materializeFinalNextConfig(userNextConfig); expect(finalConfig).toEqual( expect.objectContaining({ @@ -142,7 +142,7 @@ describe('withSentryConfig', () => { it("works when user's overall config is a function", () => { const userNextConfigFunction = () => userNextConfig; - const finalConfig = materializeFinalNextConfig(userNextConfigFunction, userSentryWebpackPluginConfig); + const finalConfig = materializeFinalNextConfig(userNextConfigFunction); expect(finalConfig).toEqual( expect.objectContaining({ @@ -157,7 +157,6 @@ describe('webpack config', () => { it('includes expected properties', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, - userSentryWebpackPluginConfig, incomingWebpackConfig: serverWebpackConfig, incomingWebpackBuildContext: serverBuildContext, }); @@ -174,7 +173,6 @@ describe('webpack config', () => { it('preserves unrelated webpack config options', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, - userSentryWebpackPluginConfig, incomingWebpackConfig: serverWebpackConfig, incomingWebpackBuildContext: serverBuildContext, }); @@ -192,7 +190,6 @@ describe('webpack config', () => { it('injects correct code when building server bundle', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, - userSentryWebpackPluginConfig, incomingWebpackConfig: serverWebpackConfig, incomingWebpackBuildContext: serverBuildContext, }); @@ -207,7 +204,6 @@ describe('webpack config', () => { it('injects correct code when building client bundle', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, - userSentryWebpackPluginConfig, incomingWebpackConfig: clientWebpackConfig, incomingWebpackBuildContext: clientBuildContext, }); @@ -221,7 +217,6 @@ describe('webpack config', () => { it('handles non-empty `main.js` entry point', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, - userSentryWebpackPluginConfig, incomingWebpackConfig: { ...clientWebpackConfig, entry: () => Promise.resolve({ main: './src/index.ts', 'main.js': ['sitLieDownRollOver.config.js'] }), From ede72005761a581bf83896d378d79c5ef9f97bbb Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Jun 2021 23:53:07 -0700 Subject: [PATCH 5/8] remove stray productionBrowserSourceMaps from next config type --- packages/nextjs/src/config/types.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts index 6ae7a52aae11..468ec1e1057b 100644 --- a/packages/nextjs/src/config/types.ts +++ b/packages/nextjs/src/config/types.ts @@ -7,9 +7,6 @@ export { SentryCliPluginOptions as SentryWebpackPluginOptions } from '@sentry/we export type ExportedNextConfig = NextConfigObject | NextConfigFunction; export type NextConfigObject = { - // whether or not next should create source maps for browser code - // see: https://nextjs.org/docs/advanced-features/source-maps - productionBrowserSourceMaps?: boolean; // custom webpack options webpack?: WebpackConfigFunction; } & { From 4846ede65b1a574b02b08bec9f1b8de8e7dec153 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 29 Jun 2021 23:55:43 -0700 Subject: [PATCH 6/8] clone config to avoid mutability issues --- packages/nextjs/src/config/webpack.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 55ac3eb68d7f..b296b1152fef 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -54,18 +54,19 @@ export function constructWebpackConfigFunction( userSentryWebpackPluginOptions: Partial = {}, ): WebpackConfigFunction { const newWebpackFunction = (config: WebpackConfigObject, options: BuildContext): WebpackConfigObject => { + // clone to avoid mutability issues + let newConfig = { ...config }; + // if we're building server code, store the webpack output path as an env variable, so we know where to look for the // webpack-processed version of `sentry.server.config.js` when we need it - if (config.target === 'node') { - storeServerConfigFileLocation(config); + if (newConfig.target === 'node') { + storeServerConfigFileLocation(newConfig); } - let newConfig = config; - // if user has custom webpack config (which always takes the form of a function), run it so we have actual values to // work with if ('webpack' in userNextConfig && typeof userNextConfig.webpack === 'function') { - newConfig = userNextConfig.webpack(config, options); + newConfig = userNextConfig.webpack(newConfig, options); } // Ensure quality source maps in production. (Source maps aren't uploaded in dev, and besides, Next doesn't let you From 55f4c51977e235da7fb85678b3cf3db45a420ce9 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 30 Jun 2021 00:00:30 -0700 Subject: [PATCH 7/8] ensure that constructWebpackConfigFunction is passed an object for userNextConfig --- packages/nextjs/src/config/index.ts | 15 ++++++++------- packages/nextjs/src/config/webpack.ts | 4 ++-- packages/nextjs/test/config.test.ts | 13 ++++++++++++- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/config/index.ts b/packages/nextjs/src/config/index.ts index e8485a2f8be4..499c96805d9d 100644 --- a/packages/nextjs/src/config/index.ts +++ b/packages/nextjs/src/config/index.ts @@ -12,17 +12,18 @@ export function withSentryConfig( userNextConfig: ExportedNextConfig = {}, userSentryWebpackPluginOptions: Partial = {}, ): NextConfigFunction | NextConfigObject { - const newWebpackConfig = constructWebpackConfigFunction(userNextConfig, userSentryWebpackPluginOptions); - // If the user has passed us a function, we need to return a function, so that we have access to `phase` and // `defaults` in order to pass them along to the user's function if (typeof userNextConfig === 'function') { - return (phase: string, defaults: { defaultConfig: { [key: string]: unknown } }): NextConfigObject => ({ - ...userNextConfig(phase, defaults), - webpack: newWebpackConfig, - }); + return function(phase: string, defaults: { defaultConfig: { [key: string]: unknown } }): NextConfigObject { + const materializedUserNextConfig = userNextConfig(phase, defaults); + return { + ...materializedUserNextConfig, + webpack: constructWebpackConfigFunction(materializedUserNextConfig, userSentryWebpackPluginOptions), + }; + }; } // Otherwise, we can just merge their config with ours and return an object. - return { ...userNextConfig, webpack: newWebpackConfig }; + return { ...userNextConfig, webpack: constructWebpackConfigFunction(userNextConfig, userSentryWebpackPluginOptions) }; } diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index b296b1152fef..9ef791b2e1a1 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -6,7 +6,7 @@ import { BuildContext, EntryPointObject, EntryPropertyObject, - ExportedNextConfig, + NextConfigObject, SentryWebpackPluginOptions, WebpackConfigFunction, WebpackConfigObject, @@ -50,7 +50,7 @@ const defaultSentryWebpackPluginOptions = dropUndefinedKeys({ * @returns The function to set as the nextjs config's `webpack` value */ export function constructWebpackConfigFunction( - userNextConfig: ExportedNextConfig = {}, + userNextConfig: NextConfigObject = {}, userSentryWebpackPluginOptions: Partial = {}, ): WebpackConfigFunction { const newWebpackFunction = (config: WebpackConfigObject, options: BuildContext): WebpackConfigObject => { diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index 1b436b1820f6..be33952077cf 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -100,8 +100,19 @@ async function materializeFinalWebpackConfig(options: { }): Promise { const { userNextConfig, userSentryWebpackPluginConfig, incomingWebpackConfig, incomingWebpackBuildContext } = options; + // if the user's next config is a function, run it so we have access to the values + const materializedUserNextConfig = + typeof userNextConfig === 'function' + ? userNextConfig('phase-production-build', { + defaultConfig: {}, + }) + : userNextConfig; + // get the webpack config function we'd normally pass back to next - const webpackConfigFunction = constructWebpackConfigFunction(userNextConfig, userSentryWebpackPluginConfig); + const webpackConfigFunction = constructWebpackConfigFunction( + materializedUserNextConfig, + userSentryWebpackPluginConfig, + ); // call it to get concrete values for comparison const finalWebpackConfigValue = webpackConfigFunction(incomingWebpackConfig, incomingWebpackBuildContext); From 5e8a39090c9f55ba933a65fff574780d1b16df77 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 30 Jun 2021 12:02:09 -0700 Subject: [PATCH 8/8] add test for next config function arguments --- packages/nextjs/test/config.test.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index be33952077cf..09abc3d94c13 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -35,6 +35,10 @@ const userNextConfig = { }; const userSentryWebpackPluginConfig = { org: 'squirrelChasers', project: 'simulator', include: './thirdPartyMaps' }; +/** mocks of the arguments passed to the result of `withSentryConfig` (when it's a function) */ +const runtimePhase = 'puppy-phase-chew-everything-in-sight'; +const defaultNextConfig = { nappingHoursPerDay: 20, oversizeFeet: true, shouldChaseTail: true }; + /** mocks of the arguments passed to `nextConfig.webpack` */ const serverWebpackConfig = { entry: () => Promise.resolve({ 'pages/api/dogs/[name]': 'private-next-pages/api/dogs/[name].js' }), @@ -71,8 +75,8 @@ function materializeFinalNextConfig( if (typeof sentrifiedConfig === 'function') { // for some reason TS won't recognize that `finalConfigValues` is now a NextConfigObject, which is why the cast // below is necessary - finalConfigValues = sentrifiedConfig('phase-production-build', { - defaultConfig: {}, + finalConfigValues = sentrifiedConfig(runtimePhase, { + defaultConfig: defaultNextConfig, }); } @@ -162,6 +166,16 @@ describe('withSentryConfig', () => { }), ); }); + + it('correctly passes `phase` and `defaultConfig` through to functional `userNextConfig`', () => { + const userNextConfigFunction = jest.fn().mockReturnValue(userNextConfig); + + materializeFinalNextConfig(userNextConfigFunction); + + expect(userNextConfigFunction).toHaveBeenCalledWith(runtimePhase, { + defaultConfig: defaultNextConfig, + }); + }); }); describe('webpack config', () => {