From 4fb043e875a50611f77ff7c1904b8e0c0fce0e2c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 21 Jun 2023 09:49:14 +0200 Subject: [PATCH] fix(nextjs): Inject init calls via loader instead of via entrypoints (#8368) --- .github/workflows/build.yml | 2 +- .../src/config/loaders/wrappingLoader.ts | 18 +-- packages/nextjs/src/config/webpack.ts | 42 +---- .../webpack/constructWebpackConfig.test.ts | 149 ------------------ 4 files changed, 18 insertions(+), 193 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 990361d6ae7c..3bc95b171d94 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -424,7 +424,7 @@ jobs: name: Nextjs (Node ${{ matrix.node }}) Tests needs: [job_get_metadata, job_build] if: needs.job_get_metadata.outputs.changed_nextjs == 'true' || github.event_name != 'pull_request' - timeout-minutes: 15 + timeout-minutes: 25 runs-on: ubuntu-20.04 strategy: fail-fast: false diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 3157d41df71f..e4d58c579420 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -201,21 +201,21 @@ export default function wrappingLoader( } else { templateCode = templateCode.replace(/__COMPONENT_TYPE__/g, 'Unknown'); } - - // We check whether `this.resourcePath` is absolute because there is no contract by webpack that says it is absolute, - // however we can only create relative paths to the sentry config from absolute paths.Examples where this could possibly be non - absolute are virtual modules. - if (sentryConfigFilePath && path.isAbsolute(this.resourcePath)) { - const sentryConfigImportPath = path - .relative(path.dirname(this.resourcePath), sentryConfigFilePath) // Absolute paths do not work with Windows: https://github.com/getsentry/sentry-javascript/issues/8133 - .replace(/\\/g, '/'); - templateCode = `import "${sentryConfigImportPath}";\n`.concat(templateCode); - } } else if (wrappingTargetKind === 'middleware') { templateCode = middlewareWrapperTemplateCode; } else { throw new Error(`Invariant: Could not get template code of unknown kind "${wrappingTargetKind}"`); } + // We check whether `this.resourcePath` is absolute because there is no contract by webpack that says it is absolute, + // however we can only create relative paths to the sentry config from absolute paths.Examples where this could possibly be non - absolute are virtual modules. + if (sentryConfigFilePath && path.isAbsolute(this.resourcePath)) { + const sentryConfigImportPath = path + .relative(path.dirname(this.resourcePath), sentryConfigFilePath) // Absolute paths do not work with Windows: https://github.com/getsentry/sentry-javascript/issues/8133 + .replace(/\\/g, '/'); + templateCode = `import "${sentryConfigImportPath}";\n`.concat(templateCode); + } + // Replace the import path of the wrapping target in the template with a path that the `wrapUserCode` function will understand. templateCode = templateCode.replace(/__SENTRY_WRAPPING_TARGET_FILE__/g, WRAPPING_TARGET_MODULE_NAME); diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 0bb42f98b7ec..7237d8ce24be 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -1,7 +1,7 @@ /* eslint-disable complexity */ /* eslint-disable max-lines */ import { getSentryRelease } from '@sentry/node'; -import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils'; +import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger } from '@sentry/utils'; import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin'; import * as chalk from 'chalk'; import * as fs from 'fs'; @@ -441,7 +441,7 @@ async function addSentryToEntryProperty( // inject into all entry points which might contain user's code for (const entryPointName in newEntryProperty) { - if (shouldAddSentryToEntryPoint(entryPointName, runtime, userSentryOptions.excludeServerRoutes ?? [])) { + if (shouldAddSentryToEntryPoint(entryPointName, runtime)) { addFilesToExistingEntryPoint(newEntryProperty, entryPointName, filesToInject); } else { if ( @@ -589,39 +589,13 @@ function checkWebpackPluginOverrides( * @param excludeServerRoutes A list of excluded serverside entrypoints provided by the user * @returns `true` if sentry code should be injected, and `false` otherwise */ -function shouldAddSentryToEntryPoint( - entryPointName: string, - runtime: 'node' | 'browser' | 'edge', - excludeServerRoutes: Array, -): boolean { - // On the server side, by default we inject the `Sentry.init()` code into every page (with a few exceptions). - if (runtime === 'node') { - // User-specified pages to skip. (Note: For ease of use, `excludeServerRoutes` is specified in terms of routes, - // which don't have the `pages` prefix.) - const entryPointRoute = entryPointName.replace(/^pages/, ''); - if (stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true)) { - return false; - } - - // This expression will implicitly include `pages/_app` which is called for all serverside routes and pages - // regardless whether or not the user has a`_app` file. - return entryPointName.startsWith('pages/'); - } else if (runtime === 'browser') { - return ( - // entrypoint for `/pages` pages - this is included on all clientside pages - // It's important that we inject the SDK into this file and not into 'main' because in 'main' - // some important Next.js code (like the setup code for getCongig()) is located and some users - // may need this code inside their Sentry configs - entryPointName === 'pages/_app' || +function shouldAddSentryToEntryPoint(entryPointName: string, runtime: 'node' | 'browser' | 'edge'): boolean { + return ( + runtime === 'browser' && + (entryPointName === 'pages/_app' || // entrypoint for `/app` pages - entryPointName === 'main-app' - ); - } else { - // User-specified pages to skip. (Note: For ease of use, `excludeServerRoutes` is specified in terms of routes, - // which don't have the `pages` prefix.) - const entryPointRoute = entryPointName.replace(/^pages/, ''); - return !stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true); - } + entryPointName === 'main-app') + ); } /** diff --git a/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts b/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts index 01f89bed1077..86bb2d03d3fd 100644 --- a/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts +++ b/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts @@ -7,10 +7,7 @@ import { CLIENT_SDK_CONFIG_FILE, clientBuildContext, clientWebpackConfig, - EDGE_SDK_CONFIG_FILE, - edgeBuildContext, exportedNextConfig, - SERVER_SDK_CONFIG_FILE, serverBuildContext, serverWebpackConfig, userNextConfig, @@ -88,74 +85,15 @@ describe('constructWebpackConfigFunction()', () => { }); describe('webpack `entry` property config', () => { - const serverConfigFilePath = `./${SERVER_SDK_CONFIG_FILE}`; const clientConfigFilePath = `./${CLIENT_SDK_CONFIG_FILE}`; - const edgeConfigFilePath = `./${EDGE_SDK_CONFIG_FILE}`; - - it('handles various entrypoint shapes', async () => { - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); - - expect(finalWebpackConfig.entry).toEqual( - expect.objectContaining({ - // original entrypoint value is a string - // (was 'private-next-pages/_error.js') - 'pages/_error': [serverConfigFilePath, 'private-next-pages/_error.js'], - - // original entrypoint value is a string array - // (was ['./node_modules/smellOVision/index.js', 'private-next-pages/sniffTour.js']) - 'pages/sniffTour': [ - serverConfigFilePath, - './node_modules/smellOVision/index.js', - 'private-next-pages/sniffTour.js', - ], - - // original entrypoint value is an object containing a string `import` value - // (was { import: 'private-next-pages/api/simulator/dogStats/[name].js' }) - 'pages/api/simulator/dogStats/[name]': { - import: [serverConfigFilePath, 'private-next-pages/api/simulator/dogStats/[name].js'], - }, - - // original entrypoint value is an object containing a string array `import` value - // (was { import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/simulator/leaderboard.js'] }) - 'pages/simulator/leaderboard': { - import: [ - serverConfigFilePath, - './node_modules/dogPoints/converter.js', - 'private-next-pages/simulator/leaderboard.js', - ], - }, - - // original entrypoint value is an object containg properties besides `import` - // (was { import: 'private-next-pages/api/tricks/[trickName].js', dependOn: 'treats', }) - 'pages/api/tricks/[trickName]': { - import: [serverConfigFilePath, 'private-next-pages/api/tricks/[trickName].js'], - dependOn: 'treats', // untouched - }, - }), - ); - }); it('injects user config file into `_app` in server bundle and in the client bundle', async () => { - const finalServerWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); const finalClientWebpackConfig = await materializeFinalWebpackConfig({ exportedNextConfig, incomingWebpackConfig: clientWebpackConfig, incomingWebpackBuildContext: clientBuildContext, }); - expect(finalServerWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/_app': expect.arrayContaining([serverConfigFilePath]), - }), - ); expect(finalClientWebpackConfig.entry).toEqual( expect.objectContaining({ 'pages/_app': expect.arrayContaining([clientConfigFilePath]), @@ -163,68 +101,6 @@ describe('constructWebpackConfigFunction()', () => { ); }); - it('injects user config file into `_error` in server bundle but not client bundle', async () => { - const finalServerWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); - const finalClientWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: clientWebpackConfig, - incomingWebpackBuildContext: clientBuildContext, - }); - - expect(finalServerWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/_error': expect.arrayContaining([serverConfigFilePath]), - }), - ); - expect(finalClientWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/_error': expect.not.arrayContaining([clientConfigFilePath]), - }), - ); - }); - - it('injects user config file into both API routes and non-API routes', async () => { - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); - - expect(finalWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/api/simulator/dogStats/[name]': { - import: expect.arrayContaining([serverConfigFilePath]), - }, - - 'pages/api/tricks/[trickName]': expect.objectContaining({ - import: expect.arrayContaining([serverConfigFilePath]), - }), - - 'pages/simulator/leaderboard': { - import: expect.arrayContaining([serverConfigFilePath]), - }, - }), - ); - }); - - it('injects user config file into API middleware', async () => { - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: edgeBuildContext, - }); - - expect(finalWebpackConfig.entry).toEqual( - expect.objectContaining({ - middleware: [edgeConfigFilePath, 'private-next-pages/middleware.js'], - }), - ); - }); - it('does not inject anything into non-_app pages during client build', async () => { const finalWebpackConfig = await materializeFinalWebpackConfig({ exportedNextConfig, @@ -244,30 +120,5 @@ describe('constructWebpackConfigFunction()', () => { simulatorBundle: './src/simulator/index.ts', }); }); - - it('does not inject into routes included in `excludeServerRoutes`', async () => { - const nextConfigWithExcludedRoutes = { - ...exportedNextConfig, - sentry: { - excludeServerRoutes: [/simulator/], - }, - }; - const finalWebpackConfig = await materializeFinalWebpackConfig({ - exportedNextConfig: nextConfigWithExcludedRoutes, - incomingWebpackConfig: serverWebpackConfig, - incomingWebpackBuildContext: serverBuildContext, - }); - - expect(finalWebpackConfig.entry).toEqual( - expect.objectContaining({ - 'pages/simulator/leaderboard': { - import: expect.not.arrayContaining([serverConfigFilePath]), - }, - 'pages/api/simulator/dogStats/[name]': { - import: expect.not.arrayContaining([serverConfigFilePath]), - }, - }), - ); - }); }); });