From a282b8c281e2c26c11895613742286c5a8216f9b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 1 Dec 2022 06:27:40 -0800 Subject: [PATCH 1/6] rename `prefixLoaderTemplate` to `serverRewriteFramesPrefixLoaderTemplate` --- packages/nextjs/rollup.npm.config.js | 2 +- ...erTemplate.ts => serverRewriteFramesPrefixLoaderTemplate.ts} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename packages/nextjs/src/config/templates/{prefixLoaderTemplate.ts => serverRewriteFramesPrefixLoaderTemplate.ts} (100%) diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index 07a8594a5b1f..204cdccb6997 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -20,7 +20,7 @@ export default [ ...makeNPMConfigVariants( makeBaseNPMConfig({ entrypoints: [ - 'src/config/templates/prefixLoaderTemplate.ts', + 'src/config/templates/serverRewriteFramesPrefixLoaderTemplate.ts', 'src/config/templates/pageProxyLoaderTemplate.ts', 'src/config/templates/apiProxyLoaderTemplate.ts', ], diff --git a/packages/nextjs/src/config/templates/prefixLoaderTemplate.ts b/packages/nextjs/src/config/templates/serverRewriteFramesPrefixLoaderTemplate.ts similarity index 100% rename from packages/nextjs/src/config/templates/prefixLoaderTemplate.ts rename to packages/nextjs/src/config/templates/serverRewriteFramesPrefixLoaderTemplate.ts From 22fab0f8abb76d0c37070c1ad0def28d0175109f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 1 Dec 2022 06:34:08 -0800 Subject: [PATCH 2/6] parameterize prefix loader template name and replacement values --- .../nextjs/src/config/loaders/prefixLoader.ts | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/nextjs/src/config/loaders/prefixLoader.ts b/packages/nextjs/src/config/loaders/prefixLoader.ts index 040163409f04..f5aa562b5290 100644 --- a/packages/nextjs/src/config/loaders/prefixLoader.ts +++ b/packages/nextjs/src/config/loaders/prefixLoader.ts @@ -1,26 +1,38 @@ +import { escapeStringForRegex } from '@sentry/utils'; import * as fs from 'fs'; import * as path from 'path'; import { LoaderThis } from './types'; type LoaderOptions = { - distDir: string; + templatePrefix: string; + replacements: Array<[string, string]>; }; /** * Inject templated code into the beginning of a module. + * + * Options: + * - `templatePrefix`: The XXX in `XXXPrefixLoaderTemplate.ts`, to specify which template to use + * - `replacements`: An array of tuples of the form `[, ]`, used for doing global + * string replacement in the template. Note: The replacement is done sequentially, in the order in which the + * replacement values are given. If any placeholder is a substring of any replacement value besides its own, make + * sure to order the tuples in such a way as to avoid over-replacement. */ export default function prefixLoader(this: LoaderThis, userCode: string): string { // We know one or the other will be defined, depending on the version of webpack being used - const { distDir } = 'getOptions' in this ? this.getOptions() : this.query; + const { templatePrefix, replacements } = 'getOptions' in this ? this.getOptions() : this.query; - const templatePath = path.resolve(__dirname, '../templates/prefixLoaderTemplate.js'); + const templatePath = path.resolve(__dirname, `../templates/${templatePrefix}PrefixLoaderTemplate.js`); // make sure the template is included when runing `webpack watch` this.addDependency(templatePath); - // Fill in the placeholder + // Fill in placeholders let templateCode = fs.readFileSync(templatePath).toString(); - templateCode = templateCode.replace('__DIST_DIR__', distDir.replace(/\\/g, '\\\\')); + replacements.forEach(([placeholder, value]) => { + const placeholderRegex = new RegExp(escapeStringForRegex(placeholder), 'g'); + templateCode = templateCode.replace(placeholderRegex, value); + }); return `${templateCode}\n${userCode}`; } From 9015847d95bdcd5a8d9cefb0edb379a2bdcc8858 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 1 Dec 2022 06:26:35 -0800 Subject: [PATCH 3/6] add `WebpackConfigObjectWithModuleRules` type --- packages/nextjs/src/config/types.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts index dc951d35aefe..90e23bd64fb9 100644 --- a/packages/nextjs/src/config/types.ts +++ b/packages/nextjs/src/config/types.ts @@ -100,6 +100,9 @@ export type WebpackConfigObject = { [key: string]: unknown; }; +// A convenience type to save us from having to assert the existence of `module.rules` over and over +export type WebpackConfigObjectWithModuleRules = WebpackConfigObject & Required>; + // Information about the current build environment export type BuildContext = { dev: boolean; From e2522de7079b3c78dbdf6ef3fd596c40c364eb9e Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 1 Dec 2022 08:14:56 -0800 Subject: [PATCH 4/6] use helper to ensure `newConfig.module.rules` exists --- packages/nextjs/src/config/webpack.ts | 54 +++++++++++++++++---------- 1 file changed, 35 insertions(+), 19 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 7e01b63ce85d..7870ed1eee9f 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -23,6 +23,7 @@ import type { UserSentryOptions, WebpackConfigFunction, WebpackConfigObject, + WebpackConfigObjectWithModuleRules, WebpackEntryProperty, WebpackModuleRule, WebpackPluginInstance, @@ -67,35 +68,33 @@ export function constructWebpackConfigFunction( buildContext: BuildContext, ): WebpackConfigObject { const { isServer, dev: isDev, dir: projectDir } = buildContext; - let newConfig = { ...incomingConfig }; + let rawNewConfig = { ...incomingConfig }; // 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(newConfig, buildContext); + rawNewConfig = userNextConfig.webpack(rawNewConfig, buildContext); } + // This mutates `rawNewConfig` in place, but also returns it in order to switch its type to one in which + // `newConfig.module.rules` is required, so we don't have to keep asserting its existence + const newConfig = setUpModuleRules(rawNewConfig); + if (isServer) { - newConfig.module = { - ...newConfig.module, - rules: [ - ...(newConfig.module?.rules || []), + newConfig.module.rules.push({ + test: /sentry\.server\.config\.(jsx?|tsx?)/, + use: [ { - test: /sentry\.server\.config\.(jsx?|tsx?)/, - use: [ - { - // Support non-default output directories by making the output path (easy to get here at build-time) - // available to the server SDK's default `RewriteFrames` instance (which needs it at runtime), by - // injecting code to attach it to `global`. - loader: path.resolve(__dirname, 'loaders/prefixLoader.js'), - options: { - distDir: userNextConfig.distDir || '.next', - }, - }, - ], + // Support non-default output directories by making the output path (easy to get here at build-time) + // available to the server SDK's default `RewriteFrames` instance (which needs it at runtime), by + // injecting code to attach it to `global`. + loader: path.resolve(__dirname, 'loaders/prefixLoader.js'), + options: { + distDir: userNextConfig.distDir || '.next', + }, }, ], - }; + }); if (userSentryOptions.autoInstrumentServerFunctions !== false) { const pagesDir = newConfig.resolve?.alias?.['private-next-pages'] as string; @@ -628,3 +627,20 @@ function handleSourcemapHidingOptionWarning(userSentryOptions: UserSentryOptions // ); // } } + +/** + * Ensure that `newConfig.module.rules` exists. Modifies the given config in place but also returns it in order to + * change its type. + * + * @param newConfig A webpack config object which may or may not contain `module` and `module.rules` + * @returns The same object, with an empty `module.rules` array added if necessary + */ +function setUpModuleRules(newConfig: WebpackConfigObject): WebpackConfigObjectWithModuleRules { + newConfig.module = { + ...newConfig.module, + rules: [...(newConfig.module?.rules || [])], + }; + // Surprising that we have to assert the type here, since we've demonstrably guaranteed the existence of + // `newConfig.module.rules` just above, but ¯\_(ツ)_/¯ + return newConfig as WebpackConfigObjectWithModuleRules; +} From f38e2c29884e4d18087b88cebd16a0065d6a06bd Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 1 Dec 2022 08:16:21 -0800 Subject: [PATCH 5/6] use helper to add `RewriteFrames` prefix loader --- packages/nextjs/src/config/webpack.ts | 55 ++++++++++++++++++++------- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 7870ed1eee9f..09427319af45 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -81,20 +81,8 @@ export function constructWebpackConfigFunction( const newConfig = setUpModuleRules(rawNewConfig); if (isServer) { - newConfig.module.rules.push({ - test: /sentry\.server\.config\.(jsx?|tsx?)/, - use: [ - { - // Support non-default output directories by making the output path (easy to get here at build-time) - // available to the server SDK's default `RewriteFrames` instance (which needs it at runtime), by - // injecting code to attach it to `global`. - loader: path.resolve(__dirname, 'loaders/prefixLoader.js'), - options: { - distDir: userNextConfig.distDir || '.next', - }, - }, - ], - }); + // This loader will inject code setting global values for use by `RewriteFrames` + addRewriteFramesLoader(newConfig, 'server', userNextConfig); if (userSentryOptions.autoInstrumentServerFunctions !== false) { const pagesDir = newConfig.resolve?.alias?.['private-next-pages'] as string; @@ -644,3 +632,42 @@ function setUpModuleRules(newConfig: WebpackConfigObject): WebpackConfigObjectWi // `newConfig.module.rules` just above, but ¯\_(ツ)_/¯ return newConfig as WebpackConfigObjectWithModuleRules; } + +/** + * Support the `distDir` option by making its value (easy to get here at build-time) available to the server SDK's + * default `RewriteFrames` instance (which needs it at runtime), by injecting code to attach it to `global`. + * + * @param newConfig The webpack config object being constructed + * @param target Either 'server' or 'client' + * @param userNextConfig The user's nextjs config options + */ +function addRewriteFramesLoader( + newConfig: WebpackConfigObjectWithModuleRules, + target: 'server' | 'client', + userNextConfig: NextConfigObject, +): void { + const replacements = { + server: [ + [ + '__DIST_DIR__', + // Make sure that if we have a windows path, the backslashes are interpreted as such (rather than as escape + // characters) + userNextConfig.distDir?.replace(/\\/g, '\\\\') || '.next', + ], + ], + }; + + newConfig.module.rules.push({ + test: new RegExp(`sentry\\.${target}\\.config\\.(jsx?|tsx?)`), + use: [ + { + loader: path.resolve(__dirname, 'loaders/prefixLoader.js'), + options: { + templatePrefix: `${target}RewriteFrames`, + // This weird cast will go away as soon as we add the client half of this function in + replacements: replacements[target as 'server'], + }, + }, + ], + }); +} From acef63cead970185b2d8d135349cbc3fd98b9dae Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 1 Dec 2022 15:39:13 -0800 Subject: [PATCH 6/6] fix tests --- packages/nextjs/test/config/loaders.test.ts | 85 +++++++++++++++------ packages/nextjs/test/config/testUtils.ts | 5 +- 2 files changed, 63 insertions(+), 27 deletions(-) diff --git a/packages/nextjs/test/config/loaders.test.ts b/packages/nextjs/test/config/loaders.test.ts index 9cdcb8c08ff5..14d1bdf9394f 100644 --- a/packages/nextjs/test/config/loaders.test.ts +++ b/packages/nextjs/test/config/loaders.test.ts @@ -10,39 +10,74 @@ import { } from './fixtures'; import { materializeFinalWebpackConfig } from './testUtils'; +type MatcherResult = { pass: boolean; message: () => string }; + +expect.extend({ + stringEndingWith(received: string, expectedEnding: string): MatcherResult { + const failsTest = !received.endsWith(expectedEnding); + const generateErrorMessage = () => + failsTest + ? // Regular error message for match failing + `expected string ending with '${expectedEnding}', but got '${received}'` + : // Error message for the match passing if someone has called it with `expect.not` + `expected string not ending with '${expectedEnding}', but got '${received}'`; + + return { + pass: !failsTest, + message: generateErrorMessage, + }; + }, +}); + +declare global { + // eslint-disable-next-line @typescript-eslint/no-namespace + namespace jest { + interface Expect { + stringEndingWith: (expectedEnding: string) => MatcherResult; + } + } +} + describe('webpack loaders', () => { - it('adds loader to server config', async () => { - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, + describe('server loaders', () => { + it('adds server `RewriteFrames` loader to server config', async () => { + const finalWebpackConfig = await materializeFinalWebpackConfig({ + exportedNextConfig, + incomingWebpackConfig: serverWebpackConfig, + incomingWebpackBuildContext: serverBuildContext, + }); + + expect(finalWebpackConfig.module.rules).toContainEqual({ + test: /sentry\.server\.config\.(jsx?|tsx?)/, + use: [ + { + loader: expect.stringEndingWith('prefixLoader.js'), + options: expect.objectContaining({ templatePrefix: 'serverRewriteFrames' }), + }, + ], + }); }); + }); + + describe('client loaders', () => { + it("doesn't add `RewriteFrames` loader to client config", async () => { + const finalWebpackConfig = await materializeFinalWebpackConfig({ + exportedNextConfig, + incomingWebpackConfig: clientWebpackConfig, + incomingWebpackBuildContext: clientBuildContext, + }); - expect(finalWebpackConfig.module!.rules).toEqual( - expect.arrayContaining([ - { - test: expect.any(RegExp), + expect(finalWebpackConfig.module.rules).not.toContainEqual( + expect.objectContaining({ use: [ { - loader: expect.any(String), - // Having no criteria for what the object contains is better than using `expect.any(Object)`, because that - // could be anything - options: expect.objectContaining({}), + loader: expect.stringEndingWith('prefixLoader.js'), + options: expect.objectContaining({ templatePrefix: expect.stringContaining('RewriteFrames') }), }, ], - }, - ]), - ); - }); - - it("doesn't add loader to client config", async () => { - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: clientWebpackConfig, - incomingWebpackBuildContext: clientBuildContext, + }), + ); }); - - expect(finalWebpackConfig.module).toBeUndefined(); }); }); diff --git a/packages/nextjs/test/config/testUtils.ts b/packages/nextjs/test/config/testUtils.ts index 70034ccaa759..889ac4bb54da 100644 --- a/packages/nextjs/test/config/testUtils.ts +++ b/packages/nextjs/test/config/testUtils.ts @@ -7,6 +7,7 @@ import { NextConfigObject, SentryWebpackPluginOptions, WebpackConfigObject, + WebpackConfigObjectWithModuleRules, } from '../../src/config/types'; import { constructWebpackConfigFunction, SentryWebpackPlugin } from '../../src/config/webpack'; import { withSentryConfig } from '../../src/config/withSentryConfig'; @@ -57,7 +58,7 @@ export async function materializeFinalWebpackConfig(options: { userSentryWebpackPluginConfig?: Partial; incomingWebpackConfig: WebpackConfigObject; incomingWebpackBuildContext: BuildContext; -}): Promise { +}): Promise { const { exportedNextConfig, userSentryWebpackPluginConfig, incomingWebpackConfig, incomingWebpackBuildContext } = options; @@ -83,7 +84,7 @@ export async function materializeFinalWebpackConfig(options: { const webpackEntryProperty = finalWebpackConfigValue.entry as EntryPropertyFunction; finalWebpackConfigValue.entry = await webpackEntryProperty(); - return finalWebpackConfigValue; + return finalWebpackConfigValue as WebpackConfigObjectWithModuleRules; } // helper function to make sure we're checking the correct plugin's data