From 8063fbb4ffb9fddbab30a8237d937e64565a4225 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 9 Jul 2021 08:37:01 -0700 Subject: [PATCH] ref(nextjs): Inject init code in `_app` and API routes (#3786) Change how we include the user's `sentry.server.config.js` and `sentry.client.config.js` code (which is what calls `Sentry.init()`) in server and browser bundles, in order both to fix apps deployed to Vercel and to eliminate some semi-gross hacks included in the current implementation. A great deal more detail in the PR description. --- packages/nextjs/src/config/utils.ts | 60 ----------- packages/nextjs/src/config/webpack.ts | 62 +++-------- packages/nextjs/src/utils/instrumentServer.ts | 13 --- packages/nextjs/test/config.test.ts | 102 +++++++++++------- 4 files changed, 76 insertions(+), 161 deletions(-) delete mode 100644 packages/nextjs/src/config/utils.ts diff --git a/packages/nextjs/src/config/utils.ts b/packages/nextjs/src/config/utils.ts deleted file mode 100644 index b9944894402c..000000000000 --- a/packages/nextjs/src/config/utils.ts +++ /dev/null @@ -1,60 +0,0 @@ -import * as fs from 'fs'; -import * as path from 'path'; - -import { WebpackConfigObject } from './types'; - -export const SENTRY_CLIENT_CONFIG_FILE = './sentry.client.config.js'; -export const SENTRY_SERVER_CONFIG_FILE = './sentry.server.config.js'; -// this is where the transpiled/bundled version of `SENTRY_SERVER_CONFIG_FILE` will end up -export const SERVER_SDK_INIT_PATH = 'sentry/initServerSDK.js'; - -/** - * Store the path to the bundled version of the user's server config file (where `Sentry.init` is called). - * - * @param config Incoming webpack configuration, passed to the `webpack` function we set in the nextjs config. - */ -export function storeServerConfigFileLocation(config: WebpackConfigObject): void { - const outputLocation = path.dirname(path.join(config.output.path, config.output.filename)); - const serverSDKInitOutputPath = path.join(outputLocation, SERVER_SDK_INIT_PATH); - const projectDir = config.context; - setRuntimeEnvVars(projectDir, { - // ex: .next/server/sentry/initServerSdk.js - SENTRY_SERVER_INIT_PATH: path.relative(projectDir, serverSDKInitOutputPath), - }); -} - -/** - * Set variables to be added to the env at runtime, by storing them in `.env.local` (which `next` automatically reads - * into memory at server startup). - * - * @param projectDir The path to the project root - * @param vars Object containing vars to set - */ -export function setRuntimeEnvVars(projectDir: string, vars: { [key: string]: string }): void { - // ensure the file exists - const envFilePath = path.join(projectDir, '.env.local'); - if (!fs.existsSync(envFilePath)) { - fs.writeFileSync(envFilePath, ''); - } - - let fileContents = fs - .readFileSync(envFilePath) - .toString() - .trim(); - - Object.entries(vars).forEach(entry => { - const [varName, value] = entry; - const envVarString = `${varName}=${value}`; - - // new entry - if (!fileContents.includes(varName)) { - fileContents = `${fileContents}\n${envVarString}`; - } - // existing entry; make sure value is up to date - else { - fileContents = fileContents.replace(new RegExp(`${varName}=\\S+`), envVarString); - } - }); - - fs.writeFileSync(envFilePath, `${fileContents.trim()}\n`); -} diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 47d4b253bac7..9e7650412766 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -4,7 +4,6 @@ import * as SentryWebpackPlugin from '@sentry/webpack-plugin'; import { BuildContext, - EntryPointObject, EntryPointValue, EntryPropertyObject, NextConfigObject, @@ -13,7 +12,6 @@ import { WebpackConfigObject, WebpackEntryProperty, } from './types'; -import { SERVER_SDK_INIT_PATH, storeServerConfigFileLocation } from './utils'; export { SentryWebpackPlugin }; @@ -21,8 +19,8 @@ export { SentryWebpackPlugin }; // TODO: merge default SentryWebpackPlugin include with their SentryWebpackPlugin include // TODO: drop merged keys from override check? `includeDefaults` option? -const CLIENT_SDK_CONFIG_FILE = './sentry.client.config.js'; -const SERVER_SDK_CONFIG_FILE = './sentry.server.config.js'; +export const CLIENT_SDK_CONFIG_FILE = './sentry.client.config.js'; +export const SERVER_SDK_CONFIG_FILE = './sentry.server.config.js'; const defaultSentryWebpackPluginOptions = dropUndefinedKeys({ url: process.env.SENTRY_URL, @@ -58,12 +56,6 @@ export function constructWebpackConfigFunction( const newWebpackFunction = (incomingConfig: WebpackConfigObject, buildContext: BuildContext): WebpackConfigObject => { let newConfig = { ...incomingConfig }; - // 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 (newConfig.target === 'node') { - storeServerConfigFileLocation(newConfig); - } - // 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') { @@ -140,39 +132,11 @@ async function addSentryToEntryProperty( const newEntryProperty = typeof currentEntryProperty === 'function' ? await currentEntryProperty() : { ...currentEntryProperty }; - // Add a new element to the `entry` array, we force webpack to create a bundle out of the user's - // `sentry.server.config.js` file and output it to `SERVER_INIT_LOCATION`. (See - // https://webpack.js.org/guides/code-splitting/#entry-points.) We do this so that the user's config file is run - // through babel (and any other processors through which next runs the rest of the user-provided code - pages, API - // routes, etc.). Specifically, we need any ESM-style `import` code to get transpiled into ES5, so that we can call - // `require()` on the resulting file when we're instrumenting the sesrver. (We can't use a dynamic import there - // because that then forces the user into a particular TS config.) - - // On the server, create a separate bundle, as there's no one entry point depended on by all the others - if (buildContext.isServer) { - // slice off the final `.js` since webpack is going to add it back in for us, and we don't want to end up with - // `.js.js` as the extension - newEntryProperty[SERVER_SDK_INIT_PATH.slice(0, -3)] = SERVER_SDK_CONFIG_FILE; - } - // On the client, it's sufficient to inject it into the `main` JS code, which is included in every browser page. - else { - addFileToExistingEntryPoint(newEntryProperty, 'main', CLIENT_SDK_CONFIG_FILE); - - // To work around a bug in nextjs, we need to ensure that the `main.js` entry is empty (otherwise it'll choose that - // over `main` and we'll lose the change we just made). In case some other library has put something into it, copy - // its contents over before emptying it out. See - // https://github.com/getsentry/sentry-javascript/pull/3696#issuecomment-863363803.) - const mainjsValue = newEntryProperty['main.js']; - if (Array.isArray(mainjsValue) && mainjsValue.length > 0) { - const mainValue = newEntryProperty.main; - - // copy the `main.js` entries over - newEntryProperty.main = Array.isArray(mainValue) - ? [...mainjsValue, ...mainValue] - : { ...(mainValue as EntryPointObject), import: [...mainjsValue, ...(mainValue as EntryPointObject).import] }; - - // nuke the entries - newEntryProperty['main.js'] = []; + const userConfigFile = buildContext.isServer ? SERVER_SDK_CONFIG_FILE : CLIENT_SDK_CONFIG_FILE; + + for (const entryPointName in newEntryProperty) { + if (entryPointName === 'pages/_app' || entryPointName.includes('pages/api')) { + addFileToExistingEntryPoint(newEntryProperty, entryPointName, userConfigFile); } } @@ -195,22 +159,20 @@ function addFileToExistingEntryPoint( const currentEntryPoint = entryProperty[entryPointName]; let newEntryPoint: EntryPointValue; - // We inject the user's client config file after the existing code so that the config file has access to - // `publicRuntimeConfig`. See https://github.com/getsentry/sentry-javascript/issues/3485 if (typeof currentEntryPoint === 'string') { - newEntryPoint = [currentEntryPoint, filepath]; + newEntryPoint = [filepath, currentEntryPoint]; } else if (Array.isArray(currentEntryPoint)) { - newEntryPoint = [...currentEntryPoint, filepath]; + newEntryPoint = [filepath, ...currentEntryPoint]; } // descriptor object (webpack 5+) else if (typeof currentEntryPoint === 'object' && 'import' in currentEntryPoint) { const currentImportValue = currentEntryPoint.import; - let newImportValue: string | string[]; + let newImportValue; if (typeof currentImportValue === 'string') { - newImportValue = [currentImportValue, filepath]; + newImportValue = [filepath, currentImportValue]; } else { - newImportValue = [...currentImportValue, filepath]; + newImportValue = [filepath, ...currentImportValue]; } newEntryPoint = { diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index da7eaf029e43..d55ec62851ac 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -5,7 +5,6 @@ import { fill, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils' import * as domain from 'domain'; import * as http from 'http'; import { default as createNextServer } from 'next'; -import * as path from 'path'; import * as querystring from 'querystring'; import * as url from 'url'; @@ -111,18 +110,6 @@ function makeWrappedHandlerGetter(origHandlerGetter: HandlerGetter): WrappedHand // Otherwise, it's just a pass-through to the original method. const wrappedHandlerGetter = async function(this: NextServer): Promise { if (!sdkSetupComplete) { - try { - // `SENTRY_SERVER_INIT_PATH` is set at build time, and points to a webpack-processed version of the user's - // `sentry.server.config.js`. Requiring it starts the SDK. - require(path.resolve(process.env.SENTRY_SERVER_INIT_PATH as string)); - } catch (err) { - // Log the error but don't bail - we still want the wrapping to happen, in case the user is doing something weird - // and manually calling `Sentry.init()` somewhere else. We log to console instead of using logger from utils - // because Sentry is not initialized. - // eslint-disable-next-line no-console - console.error(`[Sentry] Could not initialize SDK. Received error:\n${err}`); - } - // stash this in the closure so that `makeWrappedReqHandler` can use it liveServer = this.server; const serverPrototype = Object.getPrototypeOf(liveServer); diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts index 3938704efaaf..efc465e70108 100644 --- a/packages/nextjs/test/config.test.ts +++ b/packages/nextjs/test/config.test.ts @@ -7,20 +7,14 @@ import { SentryWebpackPluginOptions, WebpackConfigObject, } from '../src/config/types'; -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 -jest.mock('../src/config/utils', () => { - const original = jest.requireActual('../src/config/utils'); - return { - ...original, - // nuke this so it won't try to look for our dummy paths - storeServerConfigFileLocation: jest.fn(), - }; -}); +import { + CLIENT_SDK_CONFIG_FILE, + constructWebpackConfigFunction, + SentryWebpackPlugin, + SERVER_SDK_CONFIG_FILE, +} from '../src/config/webpack'; -/** mocks of the arguments passed to `withSentryConfig` */ +/** Mocks of the arguments passed to `withSentryConfig` */ const userNextConfig = { publicRuntimeConfig: { location: 'dogpark', activities: ['fetch', 'chasing', 'digging'] }, webpack: (config: WebpackConfigObject, _options: BuildContext) => ({ @@ -35,19 +29,36 @@ 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'; +/** 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 }; /** mocks of the arguments passed to `nextConfig.webpack` */ const serverWebpackConfig = { - entry: () => Promise.resolve({ 'pages/api/dogs/[name]': 'private-next-pages/api/dogs/[name].js' }), + entry: () => + Promise.resolve({ + 'pages/api/dogs/[name]': 'private-next-pages/api/dogs/[name].js', + 'pages/_app': ['./node_modules/smellOVision/index.js', 'private-next-pages/_app.js'], + 'pages/api/simulator/dogStats/[name]': { import: 'private-next-pages/api/simulator/dogStats/[name].js' }, + 'pages/api/simulator/leaderboard': { + import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/api/simulator/leaderboard.js'], + }, + 'pages/api/tricks/[trickName]': { + import: 'private-next-pages/api/tricks/[trickName].js', + dependOn: 'treats', + }, + treats: './node_modules/dogTreats/treatProvider.js', + }), output: { filename: '[name].js', path: '/Users/Maisey/projects/squirrelChasingSimulator/.next' }, target: 'node', context: '/Users/Maisey/projects/squirrelChasingSimulator', }; const clientWebpackConfig = { - entry: () => Promise.resolve({ main: './src/index.ts' }), + entry: () => + Promise.resolve({ + main: './src/index.ts', + 'pages/_app': 'next-client-pages-loader?page=%2F_app', + }), output: { filename: 'static/chunks/[name].js', path: '/Users/Maisey/projects/squirrelChasingSimulator/.next' }, target: 'web', context: '/Users/Maisey/projects/squirrelChasingSimulator', @@ -212,7 +223,7 @@ describe('webpack config', () => { }); describe('webpack `entry` property config', () => { - it('injects correct code when building server bundle', async () => { + it('handles various entrypoint shapes', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, incomingWebpackConfig: serverWebpackConfig, @@ -221,38 +232,53 @@ describe('webpack config', () => { expect(finalWebpackConfig.entry).toEqual( expect.objectContaining({ - [SERVER_SDK_INIT_PATH.slice(0, -3)]: SENTRY_SERVER_CONFIG_FILE, + // original entry point value is a string + // (was 'private-next-pages/api/dogs/[name].js') + 'pages/api/dogs/[name]': [SERVER_SDK_CONFIG_FILE, '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': [SERVER_SDK_CONFIG_FILE, './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: [SERVER_SDK_CONFIG_FILE, '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: [ + SERVER_SDK_CONFIG_FILE, + './node_modules/dogPoints/converter.js', + 'private-next-pages/api/simulator/leaderboard.js', + ], + }, + + // original entry point value is an object containg properties besides `import` + // (`dependOn` remains untouched) + 'pages/api/tricks/[trickName]': { + import: [SERVER_SDK_CONFIG_FILE, 'private-next-pages/api/tricks/[trickName].js'], + dependOn: 'treats', + }, }), ); }); - it('injects correct code when building client bundle', async () => { + it('does not inject into non-_app, non-API routes', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ userNextConfig, incomingWebpackConfig: clientWebpackConfig, incomingWebpackBuildContext: clientBuildContext, }); - expect(finalWebpackConfig.entry).toEqual( - expect.objectContaining({ main: ['./src/index.ts', './sentry.client.config.js'] }), - ); - }); - - // see https://github.com/getsentry/sentry-javascript/pull/3696#issuecomment-863363803 - it('handles non-empty `main.js` entry point', async () => { - const finalWebpackConfig = await materializeFinalWebpackConfig({ - userNextConfig, - incomingWebpackConfig: { - ...clientWebpackConfig, - entry: () => Promise.resolve({ main: './src/index.ts', 'main.js': ['sitLieDownRollOver.config.js'] }), - }, - incomingWebpackBuildContext: clientBuildContext, - }); - expect(finalWebpackConfig.entry).toEqual( expect.objectContaining({ - main: ['sitLieDownRollOver.config.js', './src/index.ts', './sentry.client.config.js'], - 'main.js': [], + // no injected file + main: './src/index.ts', + // was 'next-client-pages-loader?page=%2F_app' + 'pages/_app': [CLIENT_SDK_CONFIG_FILE, 'next-client-pages-loader?page=%2F_app'], }), ); });