From 6611e0a495b32946df3312a6b1d9bb4046da9ef1 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 11 Oct 2021 14:17:59 -0700 Subject: [PATCH 1/7] add `distDir` to nextjs config type --- packages/nextjs/src/config/types.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts index a75c233fdf44..77b7316ed0ca 100644 --- a/packages/nextjs/src/config/types.ts +++ b/packages/nextjs/src/config/types.ts @@ -15,6 +15,8 @@ export type NextConfigObject = { webpack: WebpackConfigFunction; // whether to build serverless functions for all pages, not just API routes target: 'server' | 'experimental-serverless-trace'; + // the output directory for the built app (defaults to ".next") + distDir: string; sentry?: { disableServerWebpackPlugin?: boolean; disableClientWebpackPlugin?: boolean; From f3bce8c013c57bd8e330eece5669dab745b6d248 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 11 Oct 2021 14:20:10 -0700 Subject: [PATCH 2/7] allow for injecting multiple files at once into webpack `entry` entries --- packages/nextjs/src/config/webpack.ts | 28 +++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 658cd15e59be..9deb8f9a04dd 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -6,7 +6,6 @@ import * as path from 'path'; import { BuildContext, - EntryPointValue, EntryPropertyObject, NextConfigObject, SentryWebpackPluginOptions, @@ -163,25 +162,25 @@ export function getUserConfigFile(projectDir: string, platform: 'server' | 'clie } /** - * Add a file to a specific element of the given `entry` webpack config property. + * Add files to a specific element of the given `entry` webpack config property. * * @param entryProperty The existing `entry` config object * @param entryPointName The key where the file should be injected - * @param filepath The path to the injected file + * @param filepaths An array of paths to the injected files */ -function addFileToExistingEntryPoint( +function addFilesToExistingEntryPoint( entryProperty: EntryPropertyObject, entryPointName: string, - filepath: string, + filepaths: string[], ): void { // can be a string, array of strings, or object whose `import` property is one of those two const currentEntryPoint = entryProperty[entryPointName]; - let newEntryPoint: EntryPointValue; + let newEntryPoint = currentEntryPoint; if (typeof currentEntryPoint === 'string') { - newEntryPoint = [filepath, currentEntryPoint]; + newEntryPoint = [...filepaths, currentEntryPoint]; } else if (Array.isArray(currentEntryPoint)) { - newEntryPoint = [filepath, ...currentEntryPoint]; + newEntryPoint = [...filepaths, ...currentEntryPoint]; } // descriptor object (webpack 5+) else if (typeof currentEntryPoint === 'object' && 'import' in currentEntryPoint) { @@ -189,25 +188,26 @@ function addFileToExistingEntryPoint( let newImportValue; if (typeof currentImportValue === 'string') { - newImportValue = [filepath, currentImportValue]; + newImportValue = [...filepaths, currentImportValue]; } else { - newImportValue = [filepath, ...currentImportValue]; + newImportValue = [...filepaths, ...currentImportValue]; } newEntryPoint = { ...currentEntryPoint, import: newImportValue, }; - } else { - // mimic the logger prefix in order to use `console.warn` (which will always be printed, regardless of SDK settings) + } + // malformed entry point (use `console.error` rather than `logger.error` because it will always be printed, regardless + // of SDK settings) + else { // eslint-disable-next-line no-console console.error( 'Sentry Logger [Error]:', - `Could not inject SDK initialization code into entry point ${entryPointName}, as it is not a recognized format.\n`, + `Could not inject SDK initialization code into entry point ${entryPointName}, as its current value is not in a recognized format.\n`, `Expected: string | Array | { [key:string]: any, import: string | Array }\n`, `Got: ${currentEntryPoint}`, ); - return; } entryProperty[entryPointName] = newEntryPoint; From ef5a0bd90946163426dabc5c465f48992a87f4e7 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 11 Oct 2021 14:20:39 -0700 Subject: [PATCH 3/7] vendor regex-escaping code --- packages/utils/src/string.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/utils/src/string.ts b/packages/utils/src/string.ts index 4722ad4f172a..771407ca7155 100644 --- a/packages/utils/src/string.ts +++ b/packages/utils/src/string.ts @@ -101,3 +101,20 @@ export function isMatchingPattern(value: string, pattern: RegExp | string): bool } return false; } + +/** + * Given a string, escape characters which have meaning in the regex grammar, such that the result is safe to feed to + * `new RegExp()`. + * + * Based on https://github.com/sindresorhus/escape-string-regexp. Vendored to a) reduce the size by skipping the runtime + * type-checking, and b) ensure it gets down-compiled for old versions of Node (the published package only supports Node + * 12+). + * + * @param regexString The string to escape + * @returns An version of the string with all special regex characters escaped + */ +export function escapeStringForRegex(regexString: string): string { + // escape the hyphen separately so we can also replace it with a unicode literal hyphen, to avoid the problems + // discussed in https://github.com/sindresorhus/escape-string-regexp/issues/20. + return regexString.replace(/[|\\{}()[\]^$+*?.]/g, '\\$&').replace(/-/g, '\\x2d'); +} From 671bcc968098d96659b6a6d1e8f0c04a9db47ea4 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 11 Oct 2021 14:50:25 -0700 Subject: [PATCH 4/7] inject `distDir` into global at build time --- packages/nextjs/src/config/webpack.ts | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 9deb8f9a04dd..5c14fd661559 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -2,6 +2,7 @@ import { getSentryRelease } from '@sentry/node'; import { dropUndefinedKeys, logger } from '@sentry/utils'; import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin'; import * as fs from 'fs'; +import * as os from 'os'; import * as path from 'path'; import { @@ -127,14 +128,31 @@ async function addSentryToEntryProperty( const newEntryProperty = typeof currentEntryProperty === 'function' ? await currentEntryProperty() : { ...currentEntryProperty }; + // `sentry.server.config.js` or `sentry.client.config.js` (or their TS equivalents) const userConfigFile = buildContext.isServer ? getUserConfigFile(buildContext.dir, 'server') : getUserConfigFile(buildContext.dir, 'client'); + // we need to turn the filename into a path so webpack can find it + const filesToInject = [`./${userConfigFile}`]; + + // 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). + if (buildContext.isServer) { + const rewriteFramesHelper = path.resolve( + fs.mkdtempSync(path.resolve(os.tmpdir(), 'sentry-')), + 'rewriteFramesHelper.js', + ); + fs.writeFileSync(rewriteFramesHelper, `global.__rewriteFramesDistDir__ = '${buildContext.config.distDir}';\n`); + // stick our helper file ahead of the user's config file so the value is in the global namespace *before* + // `Sentry.init()` is called + filesToInject.unshift(rewriteFramesHelper); + } + + // inject into all entry points which might contain user's code for (const entryPointName in newEntryProperty) { if (shouldAddSentryToEntryPoint(entryPointName)) { - // we need to turn the filename into a path so webpack can find it - addFileToExistingEntryPoint(newEntryProperty, entryPointName, `./${userConfigFile}`); + addFilesToExistingEntryPoint(newEntryProperty, entryPointName, filesToInject); } } From 8a4c43e6f84053cffc7124415c29a9fa800bdf16 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 11 Oct 2021 14:51:12 -0700 Subject: [PATCH 5/7] use stored `distDir` value in rewrite frames integration --- packages/nextjs/src/index.server.ts | 30 ++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts index e8c096b9baf5..982e7e25b6d5 100644 --- a/packages/nextjs/src/index.server.ts +++ b/packages/nextjs/src/index.server.ts @@ -1,6 +1,7 @@ import { RewriteFrames } from '@sentry/integrations'; import { configureScope, getCurrentHub, init as nodeInit, Integrations } from '@sentry/node'; -import { logger } from '@sentry/utils'; +import { escapeStringForRegex, logger } from '@sentry/utils'; +import * as path from 'path'; import { instrumentServer } from './utils/instrumentServer'; import { MetadataBuilder } from './utils/metadataBuilder'; @@ -29,7 +30,6 @@ export function init(options: NextjsOptions): void { const metadataBuilder = new MetadataBuilder(options, ['nextjs', 'node']); metadataBuilder.addSdkMetadata(); options.environment = options.environment || process.env.NODE_ENV; - // TODO capture project root and store in an env var for RewriteFrames? addServerIntegrations(options); // Right now we only capture frontend sessions for Next.js options.autoSessionTracking = false; @@ -47,18 +47,21 @@ function sdkAlreadyInitialized(): boolean { return !!hub.getClient(); } -const SOURCEMAP_FILENAME_REGEX = /^.*\/\.next\//; - -const defaultRewriteFramesIntegration = new RewriteFrames({ - iteratee: frame => { - frame.filename = frame.filename?.replace(SOURCEMAP_FILENAME_REGEX, 'app:///_next/'); - return frame; - }, -}); - -const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true }); - function addServerIntegrations(options: NextjsOptions): void { + // This value is injected at build time, based on the output directory specified in the build config + const distDirName = (global as typeof global & { __rewriteFramesDistDir__: string }).__rewriteFramesDistDir__; + // nextjs always puts the build directory at the project root level, which is also where you run `next start` from, so + // we can read in the project directory from the currently running process + const distDirAbsPath = path.resolve(process.cwd(), distDirName); + const SOURCEMAP_FILENAME_REGEX = new RegExp(escapeStringForRegex(distDirAbsPath)); + + const defaultRewriteFramesIntegration = new RewriteFrames({ + iteratee: frame => { + frame.filename = frame.filename?.replace(SOURCEMAP_FILENAME_REGEX, 'app:///_next'); + return frame; + }, + }); + if (options.integrations) { options.integrations = addIntegration(defaultRewriteFramesIntegration, options.integrations); } else { @@ -66,6 +69,7 @@ function addServerIntegrations(options: NextjsOptions): void { } if (options.tracesSampleRate !== undefined || options.tracesSampler !== undefined) { + const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true }); options.integrations = addIntegration(defaultHttpTracingIntegration, options.integrations, { Http: { keyPath: '_tracing', value: true }, }); From ea1368bd47043e22575e5a58a9f1098a42739577 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 11 Oct 2021 19:58:10 -0700 Subject: [PATCH 6/7] fix tests --- packages/nextjs/test/config.test.ts | 112 +++++++++++++++++++--- packages/nextjs/test/index.server.test.ts | 3 + 2 files changed, 101 insertions(+), 14 deletions(-) diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index dddf18bdee4c..c41a9976d17e 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -37,6 +37,33 @@ const mockExistsSync = (path: fs.PathLike) => { }; const exitsSync = jest.spyOn(fs, 'existsSync').mockImplementation(mockExistsSync); +// Make it so that all temporary folders, either created directly by tests or by the code they're testing, will go into +// one spot that we know about, which we can then clean up when we're done +const realTmpdir = jest.requireActual('os').tmpdir; +const TEMP_DIR_PATH = path.join(realTmpdir(), 'sentry-nextjs-test'); +jest.spyOn(os, 'tmpdir').mockReturnValue(TEMP_DIR_PATH); +// In theory, we should always land in the `else` here, but this saves the cases where the prior run got interrupted and +// the `afterAll` below didn't happen. +if (fs.existsSync(TEMP_DIR_PATH)) { + rimraf.sync(path.join(TEMP_DIR_PATH, '*')); +} else { + fs.mkdirSync(TEMP_DIR_PATH); +} + +afterAll(() => { + rimraf.sync(TEMP_DIR_PATH); +}); + +// In order to know what to expect in the webpack config `entry` property, we need to know the path of the temporary +// directory created when doing the file injection, so wrap the real `mkdtempSync` and store the resulting path where we +// can access it +let tempDir: string; +const realMkdtempSync = jest.requireActual('fs').mkdtempSync; +jest.spyOn(fs, 'mkdtempSync').mockImplementation(prefix => { + tempDir = realMkdtempSync(prefix); + return tempDir; +}); + /** Mocks of the arguments passed to `withSentryConfig` */ const userNextConfig: Partial = { publicRuntimeConfig: { location: 'dogpark', activities: ['fetch', 'chasing', 'digging'] }, @@ -103,7 +130,12 @@ function getBuildContext( dev: false, buildId: 'sItStAyLiEdOwN', dir: '/Users/Maisey/projects/squirrelChasingSimulator', - config: { target: 'server', ...userNextConfig } as NextConfigObject, + config: { + // nextjs's default values + target: 'server', + distDir: '.next', + ...userNextConfig, + } as NextConfigObject, webpack: { version: webpackVersion }, isServer: buildTarget === 'server', }; @@ -279,26 +311,34 @@ describe('webpack config', () => { incomingWebpackBuildContext: serverBuildContext, }); + const rewriteFramesHelper = path.join(tempDir, 'rewriteFramesHelper.js'); + expect(finalWebpackConfig.entry).toEqual( expect.objectContaining({ // original entry point value is a string // (was 'private-next-pages/api/dogs/[name].js') - 'pages/api/dogs/[name]': [serverConfigFilePath, 'private-next-pages/api/dogs/[name].js'], + 'pages/api/dogs/[name]': [rewriteFramesHelper, serverConfigFilePath, 'private-next-pages/api/dogs/[name].js'], // original entry point value is a string array // (was ['./node_modules/smellOVision/index.js', 'private-next-pages/_app.js']) - 'pages/_app': [serverConfigFilePath, './node_modules/smellOVision/index.js', 'private-next-pages/_app.js'], + 'pages/_app': [ + rewriteFramesHelper, + serverConfigFilePath, + './node_modules/smellOVision/index.js', + 'private-next-pages/_app.js', + ], // original entry point value is an object containing a string `import` value // (`import` was 'private-next-pages/api/simulator/dogStats/[name].js') 'pages/api/simulator/dogStats/[name]': { - import: [serverConfigFilePath, 'private-next-pages/api/simulator/dogStats/[name].js'], + import: [rewriteFramesHelper, serverConfigFilePath, 'private-next-pages/api/simulator/dogStats/[name].js'], }, // original entry point value is an object containing a string array `import` value // (`import` was ['./node_modules/dogPoints/converter.js', 'private-next-pages/api/simulator/leaderboard.js']) 'pages/api/simulator/leaderboard': { import: [ + rewriteFramesHelper, serverConfigFilePath, './node_modules/dogPoints/converter.js', 'private-next-pages/api/simulator/leaderboard.js', @@ -308,14 +348,14 @@ describe('webpack config', () => { // original entry point value is an object containg properties besides `import` // (`dependOn` remains untouched) 'pages/api/tricks/[trickName]': { - import: [serverConfigFilePath, 'private-next-pages/api/tricks/[trickName].js'], + import: [rewriteFramesHelper, serverConfigFilePath, 'private-next-pages/api/tricks/[trickName].js'], dependOn: 'treats', }, }), ); }); - it('does not inject into non-_app, non-API routes', async () => { + it('does not inject anything into non-_app, non-API routes', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, incomingWebpackConfig: clientWebpackConfig, @@ -326,12 +366,60 @@ describe('webpack config', () => { expect.objectContaining({ // no injected file main: './src/index.ts', - // was 'next-client-pages-loader?page=%2F_app' + }), + ); + }); + + it('does not inject `RewriteFrames` helper into client routes', async () => { + const finalWebpackConfig = await materializeFinalWebpackConfig({ + userNextConfig, + incomingWebpackConfig: clientWebpackConfig, + incomingWebpackBuildContext: clientBuildContext, + }); + + expect(finalWebpackConfig.entry).toEqual( + expect.objectContaining({ + // was 'next-client-pages-loader?page=%2F_app', and now has client config but not`RewriteFrames` helper injected 'pages/_app': [clientConfigFilePath, 'next-client-pages-loader?page=%2F_app'], }), ); }); }); + + describe('`distDir` value in default server-side `RewriteFrames` integration', () => { + it.each([ + ['no custom `distDir`', undefined, '.next'], + ['custom `distDir`', 'dist', 'dist'], + ])( + 'creates file injecting `distDir` value into `global` - %s', + async (_name, customDistDir, expectedInjectedValue) => { + // Note: the fact that the file tested here gets injected correctly is covered in the 'webpack `entry` property + // config' tests above + + const userNextConfigDistDir = { + ...userNextConfig, + ...(customDistDir && { distDir: customDistDir }), + }; + await materializeFinalWebpackConfig({ + userNextConfig: userNextConfigDistDir, + incomingWebpackConfig: serverWebpackConfig, + incomingWebpackBuildContext: getBuildContext('server', userNextConfigDistDir), + }); + const rewriteFramesHelper = path.join(tempDir, 'rewriteFramesHelper.js'); + + expect(fs.existsSync(rewriteFramesHelper)).toBe(true); + + const injectedCode = fs.readFileSync(rewriteFramesHelper).toString(); + expect(injectedCode).toEqual(`global.__rewriteFramesDistDir__ = '${expectedInjectedValue}';\n`); + }, + ); + + describe('`RewriteFrames` ends up with correct `distDir` value', () => { + // TODO: this, along with any number of other parts of the build process, should be tested with an integration + // test which actually runs webpack and inspects the resulting bundles (and that integration test should test + // custom `distDir` values with and without a `.`, to make sure the regex escaping is working) + }); + }); }); describe('Sentry webpack plugin config', () => { @@ -580,19 +668,15 @@ describe('Sentry webpack plugin config', () => { }); describe('getUserConfigFile', () => { - let tempDir: string; - beforeAll(() => { exitsSync.mockImplementation(realExistsSync); }); beforeEach(() => { + // these will get cleaned up by the file's overall `afterAll` function, and the `mkdtempSync` mock above ensures + // that the location of the created folder is stored in `tempDir` const tempDirPathPrefix = path.join(os.tmpdir(), 'sentry-nextjs-test-'); - tempDir = fs.mkdtempSync(tempDirPathPrefix); - }); - - afterEach(() => { - rimraf.sync(tempDir); + fs.mkdtempSync(tempDirPathPrefix); }); afterAll(() => { diff --git a/packages/nextjs/test/index.server.test.ts b/packages/nextjs/test/index.server.test.ts index 2dfccef96553..972e4ed57336 100644 --- a/packages/nextjs/test/index.server.test.ts +++ b/packages/nextjs/test/index.server.test.ts @@ -10,6 +10,9 @@ const { Integrations } = SentryNode; const global = getGlobalObject(); +// normally this is set as part of the build process, so mock it here +(global as typeof global & { __rewriteFramesDistDir__: string }).__rewriteFramesDistDir__ = '.next'; + let configureScopeCallback: (scope: Scope) => void = () => undefined; jest.spyOn(SentryNode, 'configureScope').mockImplementation(callback => (configureScopeCallback = callback)); const nodeInit = jest.spyOn(SentryNode, 'init'); From c321ade567c73f5f62f1b175cf05603330777f69 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 12 Oct 2021 12:38:32 -0700 Subject: [PATCH 7/7] apply code review suggestions --- packages/nextjs/src/index.server.ts | 4 +++- packages/nextjs/test/config.test.ts | 15 ++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts index 982e7e25b6d5..1578085c25a6 100644 --- a/packages/nextjs/src/index.server.ts +++ b/packages/nextjs/src/index.server.ts @@ -14,6 +14,8 @@ export * from '@sentry/node'; // because or SSR of next.js we can only use this. export { ErrorBoundary, withErrorBoundary } from '@sentry/react'; +type GlobalWithDistDir = typeof global & { __rewriteFramesDistDir__: string }; + /** Inits the Sentry NextJS SDK on node. */ export function init(options: NextjsOptions): void { if (options.debug) { @@ -49,7 +51,7 @@ function sdkAlreadyInitialized(): boolean { function addServerIntegrations(options: NextjsOptions): void { // This value is injected at build time, based on the output directory specified in the build config - const distDirName = (global as typeof global & { __rewriteFramesDistDir__: string }).__rewriteFramesDistDir__; + const distDirName = (global as GlobalWithDistDir).__rewriteFramesDistDir__ || '.next'; // nextjs always puts the build directory at the project root level, which is also where you run `next start` from, so // we can read in the project directory from the currently running process const distDirAbsPath = path.resolve(process.cwd(), distDirName); diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index c41a9976d17e..76b9af438c0c 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -57,11 +57,10 @@ afterAll(() => { // In order to know what to expect in the webpack config `entry` property, we need to know the path of the temporary // directory created when doing the file injection, so wrap the real `mkdtempSync` and store the resulting path where we // can access it -let tempDir: string; -const realMkdtempSync = jest.requireActual('fs').mkdtempSync; -jest.spyOn(fs, 'mkdtempSync').mockImplementation(prefix => { - tempDir = realMkdtempSync(prefix); - return tempDir; +const mkdtempSyncSpy = jest.spyOn(fs, 'mkdtempSync'); + +afterEach(() => { + mkdtempSyncSpy.mockClear(); }); /** Mocks of the arguments passed to `withSentryConfig` */ @@ -311,6 +310,7 @@ describe('webpack config', () => { incomingWebpackBuildContext: serverBuildContext, }); + const tempDir = mkdtempSyncSpy.mock.results[0].value; const rewriteFramesHelper = path.join(tempDir, 'rewriteFramesHelper.js'); expect(finalWebpackConfig.entry).toEqual( @@ -405,6 +405,8 @@ describe('webpack config', () => { incomingWebpackConfig: serverWebpackConfig, incomingWebpackBuildContext: getBuildContext('server', userNextConfigDistDir), }); + + const tempDir = mkdtempSyncSpy.mock.results[0].value; const rewriteFramesHelper = path.join(tempDir, 'rewriteFramesHelper.js'); expect(fs.existsSync(rewriteFramesHelper)).toBe(true); @@ -668,6 +670,8 @@ describe('Sentry webpack plugin config', () => { }); describe('getUserConfigFile', () => { + let tempDir: string; + beforeAll(() => { exitsSync.mockImplementation(realExistsSync); }); @@ -677,6 +681,7 @@ describe('Sentry webpack plugin config', () => { // that the location of the created folder is stored in `tempDir` const tempDirPathPrefix = path.join(os.tmpdir(), 'sentry-nextjs-test-'); fs.mkdtempSync(tempDirPathPrefix); + tempDir = mkdtempSyncSpy.mock.results[0].value; }); afterAll(() => {