Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ref(nextjs): Invert serverside injection criteria #6206

Merged
merged 2 commits into from Nov 15, 2022
Merged

ref(nextjs): Invert serverside injection criteria #6206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
adf6f3c
inject sentry into all serverside entrypoints besides `_app` and `_do…
lobsterkatie Nov 15, 2022
3eeff43
fix tests for injection into entrypoints
lobsterkatie Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 31 additions & 5 deletions packages/nextjs/src/config/webpack.ts
View file
Open in desktop
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).
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
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') ||
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
// 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';
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
}
}

/**
Expand Down
11 changes: 8 additions & 3 deletions packages/nextjs/test/config/fixtures.ts
View file
Open in desktop
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
View file
Open in desktop
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',
});
});
});
});