Skip to content

Commit

Permalink
fix(nextjs): Inject init calls via loader instead of via entrypoints (#…
Browse files Browse the repository at this point in the history 
…8368)
  • Loading branch information
@lforst
lforst committed Jun 21, 2023
1 parent b84c23f commit 4fb043e
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 193 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 9 additions & 9 deletions packages/nextjs/src/config/loaders/wrappingLoader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
42 changes: 8 additions & 34 deletions packages/nextjs/src/config/webpack.ts
View file
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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 (
Expand Down Expand Up @@ -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<string | RegExp>,
): 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')
);
}

/**
Expand Down
149 changes: 0 additions & 149 deletions packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@ import {
CLIENT_SDK_CONFIG_FILE,
clientBuildContext,
clientWebpackConfig,
EDGE_SDK_CONFIG_FILE,
edgeBuildContext,
exportedNextConfig,
SERVER_SDK_CONFIG_FILE,
serverBuildContext,
serverWebpackConfig,
userNextConfig,
Expand Down Expand Up @@ -88,143 +85,22 @@ 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]),
}),
);
});

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,
Expand All @@ -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]),
},
}),
);
});
});
});

0 comments on commit 4fb043e

Please sign in to comment.