From 9703ce27b7b4db50d61c5af628c12e06b9cef390 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 31 Aug 2021 08:18:55 -0700 Subject: [PATCH] ref(nextjs): Small improvements to config tests (#3938) In the process of reviewing https://github.com/getsentry/sentry-javascript/pull/3922, I realized that there was some cleanup work I could do to make that PR work better. Specifically: - `withSentryConfig` had an incorrect type for one of the arguments to the `userNextConfig` function. - The mock for that same argument was missing its outer layer, and therefore wasn't really what it said it was. - In real life, the `config` property of `buildContext` is equal to the combination of next's default config and the user-provided config. The mock wasn't reflecting this. - That PR was having to compensate for the fact that the mocks for build context were static rather than dynamic, even though those test cases especially illustrate the ways in which the value of the mock needs to change depending on other values in the test. Where necessary, the build context is now calculated as part of the test. --- packages/nextjs/src/config/index.ts | 2 +- packages/nextjs/test/config.test.ts | 58 ++++++++++++++++------------- 2 files changed, 33 insertions(+), 27 deletions(-) diff --git a/packages/nextjs/src/config/index.ts b/packages/nextjs/src/config/index.ts index 499c96805d9d..5cc4d5964893 100644 --- a/packages/nextjs/src/config/index.ts +++ b/packages/nextjs/src/config/index.ts @@ -15,7 +15,7 @@ export function withSentryConfig( // 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 function(phase: string, defaults: { defaultConfig: { [key: string]: unknown } }): NextConfigObject { + return function(phase: string, defaults: { defaultConfig: NextConfigObject }): NextConfigObject { const materializedUserNextConfig = userNextConfig(phase, defaults); return { ...materializedUserNextConfig, diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index 29e575452ed6..eda16631d1a0 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -33,7 +33,7 @@ const mockExistsSync = (path: fs.PathLike) => { const exitsSync = jest.spyOn(fs, 'existsSync').mockImplementation(mockExistsSync); /** Mocks of the arguments passed to `withSentryConfig` */ -const userNextConfig = { +const userNextConfig: Partial = { publicRuntimeConfig: { location: 'dogpark', activities: ['fetch', 'chasing', 'digging'] }, webpack: (config: WebpackConfigObject, _options: BuildContext) => ({ ...config, @@ -51,7 +51,9 @@ process.env.SENTRY_RELEASE = 'doGsaREgReaT'; /** Mocks of the arguments passed to the result of `withSentryConfig` (when it's a function). */ const runtimePhase = 'ball-fetching'; -const defaultNextConfig = { nappingHoursPerDay: 20, oversizeFeet: true, shouldChaseTail: true }; +// `defaultConfig` is the defaults for all nextjs options (we don't use these at all in the tests, so for our purposes +// here the values don't matter) +const defaultsObject = { defaultConfig: {} }; /** mocks of the arguments passed to `nextConfig.webpack` */ const serverWebpackConfig = { @@ -84,15 +86,21 @@ const clientWebpackConfig = { context: '/Users/Maisey/projects/squirrelChasingSimulator', }; -const baseBuildContext = { - dev: false, - buildId: 'sItStAyLiEdOwN', - dir: '/Users/Maisey/projects/squirrelChasingSimulator', - config: { target: 'server' as const }, - webpack: { version: '5.4.15' }, -}; -const serverBuildContext = { isServer: true, ...baseBuildContext }; -const clientBuildContext = { isServer: false, ...baseBuildContext }; +// In real life, next will copy the `userNextConfig` into the `buildContext`. Since we're providing mocks for both of +// those, we need to mimic that behavior, and since `userNextConfig` can vary per test, we need to have the option do it +// dynamically. +function getBuildContext(buildTarget: 'server' | 'client', userNextConfig: Partial): BuildContext { + return { + dev: false, + buildId: 'sItStAyLiEdOwN', + dir: '/Users/Maisey/projects/squirrelChasingSimulator', + config: { target: 'server', ...userNextConfig }, + webpack: { version: '5.4.15' }, + isServer: buildTarget === 'server', + }; +} +const serverBuildContext = getBuildContext('server', userNextConfig); +const clientBuildContext = getBuildContext('client', userNextConfig); /** * Derive the final values of all next config options, by first applying `withSentryConfig` and then, if it returns a @@ -114,9 +122,7 @@ 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(runtimePhase, { - defaultConfig: defaultNextConfig, - }); + finalConfigValues = sentrifiedConfig(runtimePhase, defaultsObject); } return finalConfigValues as NextConfigObject; @@ -145,11 +151,7 @@ async function materializeFinalWebpackConfig(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; + typeof userNextConfig === 'function' ? userNextConfig('phase-production-build', defaultsObject) : userNextConfig; // get the webpack config function we'd normally pass back to next const webpackConfigFunction = constructWebpackConfigFunction( @@ -211,9 +213,7 @@ describe('withSentryConfig', () => { materializeFinalNextConfig(userNextConfigFunction); - expect(userNextConfigFunction).toHaveBeenCalledWith(runtimePhase, { - defaultConfig: defaultNextConfig, - }); + expect(userNextConfigFunction).toHaveBeenCalledWith(runtimePhase, defaultsObject); }); }); @@ -243,7 +243,7 @@ describe('webpack config', () => { // 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, serverBuildContext); + 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; @@ -377,10 +377,13 @@ describe('Sentry webpack plugin config', () => { }); it('has the correct value when building serverless server bundles', async () => { + const userNextConfigServerless = { ...userNextConfig }; + userNextConfigServerless.target = 'experimental-serverless-trace'; + const finalWebpackConfig = await materializeFinalWebpackConfig({ - userNextConfig, + userNextConfig: userNextConfigServerless, incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: { ...serverBuildContext, config: { target: 'experimental-serverless-trace' } }, + incomingWebpackBuildContext: getBuildContext('server', userNextConfigServerless), }); const sentryWebpackPlugin = finalWebpackConfig.plugins?.[0] as SentryWebpackPluginType; @@ -391,10 +394,13 @@ describe('Sentry webpack plugin config', () => { }); it('has the correct value when building serverful server bundles using webpack 4', async () => { + const serverBuildContextWebpack4 = getBuildContext('server', userNextConfig); + serverBuildContextWebpack4.webpack.version = '4.15.13'; + const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: { ...serverBuildContext, webpack: { version: '4.15.13' } }, + incomingWebpackBuildContext: serverBuildContextWebpack4, }); const sentryWebpackPlugin = finalWebpackConfig.plugins?.[0] as SentryWebpackPluginType;