Skip to content

Commit

Permalink
ref(nextjs): Invert serverside injection criteria (#6206)
Browse files Browse the repository at this point in the history
In nextjs, all non-API pages (other than sometimes `_error`)  include the `_app` component. Up until now, we've leveraged this fact when deciding which webpack entrypoints should have `sentry.server.config.js` injected during serverside build; specifically, we inject into `_app`, `_error`, and all API handlers, but not any other non-API pages.

This works fine, but it means that if we want to be able to pick and choose _which_ non-API pages get the config file injected, we're out of luck. Either we inject into `_app` (and therefore all non-API pages) or we don't (and therefore no non-API pages). In order to allow selective injection (which will be included in an upcoming PR), this inverts the logic: instead of `_app` being the only non-API page into which we inject, it is now one of the only pages we _don't_ inject into. (The other is `_document`, which plays a similar role as `_app` does.) Given that `_app` and `_document` can't stand on their own (without the context of a page component inside of them), they don't need to have Sentry injected separately. Having it injected into the pages `_app` and `_document` wrap is sufficient.

(Note that this change only applies to serverside injection. Client-side, we still only inject into `_app`, as we can't be selective about which pages get instrumented on the front end given all of the global monkeypatching we do.)
  • Loading branch information
lobsterkatie committed Nov 15, 2022
1 parent 6627882 commit 66bcbd7
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 27 deletions.
36 changes: 31 additions & 5 deletions packages/nextjs/src/config/webpack.ts
Expand Up @@ -380,11 +380,37 @@ function checkWebpackPluginOverrides(
* @returns `true` if sentry code should be injected, and `false` otherwise
*/
function shouldAddSentryToEntryPoint(entryPointName: string, isServer: boolean): boolean {
return (
entryPointName === 'pages/_app' ||
(entryPointName.includes('pages/api') && !entryPointName.includes('_middleware')) ||
(isServer && entryPointName === 'pages/_error')
);
// On the server side, by default we inject the `Sentry.init()` code into every page (with a few exceptions).
if (isServer) {
const entryPointRoute = entryPointName.replace(/^pages/, '');
if (
// All non-API pages contain both of these components, and we don't want to inject more than once, so as long as
// we're doing the individual pages, it's fine to skip these. (Note: Even if a given user doesn't have either or
// both of these in their `pages/` folder, they'll exist as entrypoints because nextjs will supply default
// versions.)
entryPointRoute === '/_app' ||
entryPointRoute === '/_document' ||
// While middleware was in beta, it could be anywhere (at any level) in the `pages` directory, and would be called
// `_middleware.js`. Until the SDK runs successfully in the lambda edge environment, we have to exclude these.
entryPointName.includes('_middleware') ||
// Newer versions of nextjs are starting to introduce things outside the `pages/` folder (middleware, an `app/`
// directory, etc), but until those features are stable and we know how we want to support them, the safest bet is
// not to inject anywhere but inside `pages/`.
!entryPointName.startsWith('pages/')
) {
return false;
}

// We want to inject Sentry into all other pages
return true;
}

// On the client side, we only want to inject into `_app`, because that guarantees there'll be only one copy of the
// SDK in the eventual bundle. Since `_app` is the (effectively) the root component for every nextjs app, inclusing
// Sentry there means it will be available for every front end page.
else {
return entryPointName === 'pages/_app';
}
}

/**
Expand Down
11 changes: 8 additions & 3 deletions packages/nextjs/test/config/fixtures.ts
Expand Up @@ -41,11 +41,12 @@ export const serverWebpackConfig: WebpackConfigObject = {
entry: () =>
Promise.resolve({
'pages/_error': 'private-next-pages/_error.js',
'pages/_app': ['./node_modules/smellOVision/index.js', 'private-next-pages/_app.js'],
'pages/_app': 'private-next-pages/_app.js',
'pages/sniffTour': ['./node_modules/smellOVision/index.js', 'private-next-pages/sniffTour.js'],
'pages/api/_middleware': 'private-next-pages/api/_middleware.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/simulator/leaderboard': {
import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/simulator/leaderboard.js'],
},
'pages/api/tricks/[trickName]': {
import: 'private-next-pages/api/tricks/[trickName].js',
Expand All @@ -64,6 +65,10 @@ export const clientWebpackConfig: WebpackConfigObject = {
main: './src/index.ts',
'pages/_app': 'next-client-pages-loader?page=%2F_app',
'pages/_error': 'next-client-pages-loader?page=%2F_error',
'pages/sniffTour': ['./node_modules/smellOVision/index.js', 'private-next-pages/sniffTour.js'],
'pages/simulator/leaderboard': {
import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/simulator/leaderboard.js'],
},
}),
output: { filename: 'static/chunks/[name].js', path: '/Users/Maisey/projects/squirrelChasingSimulator/.next' },
target: 'web',
Expand Down
47 changes: 28 additions & 19 deletions packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts
Expand Up @@ -102,8 +102,12 @@ describe('constructWebpackConfigFunction()', () => {
'pages/_error': [serverConfigFilePath, 'private-next-pages/_error.js'],

// original entrypoint 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'],
// (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' })
Expand All @@ -112,12 +116,12 @@ describe('constructWebpackConfigFunction()', () => {
},

// original entrypoint value is an object containing a string array `import` value
// (was { import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/api/simulator/leaderboard.js'] })
'pages/api/simulator/leaderboard': {
// (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/api/simulator/leaderboard.js',
'private-next-pages/simulator/leaderboard.js',
],
},

Expand All @@ -131,7 +135,7 @@ describe('constructWebpackConfigFunction()', () => {
);
});

it('injects user config file into `_app` in both server and client bundles', async () => {
it('injects user config file into `_app` in client bundle but not in server bundle', async () => {
const finalServerWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: serverWebpackConfig,
Expand All @@ -145,7 +149,7 @@ describe('constructWebpackConfigFunction()', () => {

expect(finalServerWebpackConfig.entry).toEqual(
expect.objectContaining({
'pages/_app': expect.arrayContaining([serverConfigFilePath]),
'pages/_app': expect.not.arrayContaining([serverConfigFilePath]),
}),
);
expect(finalClientWebpackConfig.entry).toEqual(
Expand Down Expand Up @@ -179,7 +183,7 @@ describe('constructWebpackConfigFunction()', () => {
);
});

it('injects user config file into API routes', async () => {
it('injects user config file into both API routes and non-API routes', async () => {
const finalWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: serverWebpackConfig,
Expand All @@ -192,13 +196,13 @@ describe('constructWebpackConfigFunction()', () => {
import: expect.arrayContaining([serverConfigFilePath]),
},

'pages/api/simulator/leaderboard': {
import: expect.arrayContaining([serverConfigFilePath]),
},

'pages/api/tricks/[trickName]': expect.objectContaining({
import: expect.arrayContaining([serverConfigFilePath]),
}),

'pages/simulator/leaderboard': {
import: expect.arrayContaining([serverConfigFilePath]),
},
}),
);
});
Expand All @@ -218,19 +222,24 @@ describe('constructWebpackConfigFunction()', () => {
);
});

it('does not inject anything into non-_app, non-_error, non-API routes', async () => {
it('does not inject anything into non-_app pages during client build', async () => {
const finalWebpackConfig = await materializeFinalWebpackConfig({
exportedNextConfig,
incomingWebpackConfig: clientWebpackConfig,
incomingWebpackBuildContext: clientBuildContext,
});

expect(finalWebpackConfig.entry).toEqual(
expect.objectContaining({
// no injected file
main: './src/index.ts',
}),
);
expect(finalWebpackConfig.entry).toEqual({
main: './src/index.ts',
// only _app has config file injected
'pages/_app': [clientConfigFilePath, 'next-client-pages-loader?page=%2F_app'],
'pages/_error': 'next-client-pages-loader?page=%2F_error',
'pages/sniffTour': ['./node_modules/smellOVision/index.js', 'private-next-pages/sniffTour.js'],
'pages/simulator/leaderboard': {
import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/simulator/leaderboard.js'],
},
simulatorBundle: './src/simulator/index.ts',
});
});
});
});

0 comments on commit 66bcbd7

Please sign in to comment.