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.

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

feat(nextjs): Add excludeServerRoutes config option #6207

Merged
merged 7 commits into from Nov 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 12 additions & 2 deletions packages/nextjs/src/config/loaders/proxyLoader.ts
@@ -1,4 +1,4 @@
import { escapeStringForRegex, logger } from '@sentry/utils';
import { escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils';
import * as fs from 'fs';
import * as path from 'path';

Expand All @@ -8,6 +8,7 @@ import { LoaderThis } from './types';
type LoaderOptions = {
pagesDir: string;
pageExtensionRegex: string;
excludeServerRoutes: Array<RegExp | string>;
};

/**
Expand All @@ -17,7 +18,11 @@ type LoaderOptions = {
*/
export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userCode: string): Promise<string> {
// We know one or the other will be defined, depending on the version of webpack being used
const { pagesDir, pageExtensionRegex } = 'getOptions' in this ? this.getOptions() : this.query;
const {
pagesDir,
pageExtensionRegex,
excludeServerRoutes = [],
} = 'getOptions' in this ? this.getOptions() : this.query;

// Get the parameterized route name from this page's filepath
const parameterizedRoute = path
Expand All @@ -34,6 +39,11 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
// homepage), sub back in the root route
.replace(/^$/, '/');

// Skip explicitly-ignored pages
if (stringMatchesSomePattern(parameterizedRoute, excludeServerRoutes, true)) {
return userCode;
}

// We don't want to wrap twice (or infinitely), so in the proxy we add this query string onto references to the
// wrapped file, so that we know that it's already been processed. (Adding this query string is also necessary to
// convince webpack that it's a different file than the one it's in the middle of loading now, so that the originals
Expand Down
8 changes: 8 additions & 0 deletions packages/nextjs/src/config/types.ts
Expand Up @@ -59,6 +59,14 @@ export type UserSentryOptions = {

// Automatically instrument Next.js data fetching methods and Next.js API routes
autoInstrumentServerFunctions?: boolean;

// Exclude certain serverside API routes or pages from being instrumented with Sentry. This option takes an array of
// strings or regular expressions.
//
// NOTE: Pages should be specified as routes (`/animals` or `/api/animals/[animalType]/habitat`), not filepaths
// (`pages/animals/index.js` or `.\src\pages\api\animals\[animalType]\habitat.tsx`), and strings must be be a full,
// exact match.
Comment on lines +63 to +68
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: Just something we should maybe think about: I am wondering if we should have users include "pages/" in the strings/regexes they provide here just to future-proof us against Next.js 13's app/ folder. However, the more I think about it it shouldn't matter, as the routes in the app/ folder will also just be routes like we have in the pages folder and I believe they can't collide - so we're probably good here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good question, but I think you're right, a route is a route and you can't have two with the same route path. I also think that it's better that we abstract away the underlying file structure. That way, as users do move their routes from pages/ to app/, they won't have to come and update this setting.

excludeServerRoutes?: Array<RegExp | string>;
};

export type NextConfigFunction = (phase: string, defaults: { defaultConfig: NextConfigObject }) => NextConfigObject;
Expand Down
36 changes: 30 additions & 6 deletions packages/nextjs/src/config/webpack.ts
@@ -1,6 +1,6 @@
/* eslint-disable max-lines */
import { getSentryRelease } from '@sentry/node';
import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger } from '@sentry/utils';
import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger, stringMatchesSomePattern } 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 @@ -91,7 +91,11 @@ export function constructWebpackConfigFunction(
use: [
{
loader: path.resolve(__dirname, 'loaders/proxyLoader.js'),
options: { pagesDir, pageExtensionRegex },
options: {
pagesDir,
pageExtensionRegex,
excludeServerRoutes: userSentryOptions.excludeServerRoutes,
},
},
],
});
Expand Down Expand Up @@ -135,7 +139,7 @@ export function constructWebpackConfigFunction(
// will call the callback which will call `f` which will call `x.y`... and on and on. Theoretically this could also
// be fixed by using `bind`, but this is way simpler.)
const origEntryProperty = newConfig.entry;
newConfig.entry = async () => addSentryToEntryProperty(origEntryProperty, buildContext);
newConfig.entry = async () => addSentryToEntryProperty(origEntryProperty, buildContext, userSentryOptions);

// Enable the Sentry plugin (which uploads source maps to Sentry when not in dev) by default
if (shouldEnableWebpackPlugin(buildContext, userSentryOptions)) {
Expand Down Expand Up @@ -248,6 +252,7 @@ function findTranspilationRules(rules: WebpackModuleRule[] | undefined, projectD
async function addSentryToEntryProperty(
currentEntryProperty: WebpackEntryProperty,
buildContext: BuildContext,
userSentryOptions: UserSentryOptions,
): Promise<EntryPropertyObject> {
// The `entry` entry in a webpack config can be a string, array of strings, object, or function. By default, nextjs
// sets it to an async function which returns the promise of an object of string arrays. Because we don't know whether
Expand All @@ -268,8 +273,18 @@ async function addSentryToEntryProperty(

// inject into all entry points which might contain user's code
for (const entryPointName in newEntryProperty) {
if (shouldAddSentryToEntryPoint(entryPointName, isServer)) {
if (shouldAddSentryToEntryPoint(entryPointName, isServer, userSentryOptions.excludeServerRoutes)) {
addFilesToExistingEntryPoint(newEntryProperty, entryPointName, filesToInject);
} else {
if (
isServer &&
// If the user has asked to exclude pages, confirm for them that it's worked
userSentryOptions.excludeServerRoutes &&
// We always skip these, so it's not worth telling the user that we've done so
!['pages/_app', 'pages/_document'].includes(entryPointName)
) {
__DEBUG_BUILD__ && logger.log(`Skipping Sentry injection for ${entryPointName.replace(/^pages/, '')}`);
}
}
}

Expand Down Expand Up @@ -377,13 +392,21 @@ function checkWebpackPluginOverrides(
*
* @param entryPointName The name of the entry point in question
* @param isServer Whether or not this function is being called in the context of a server build
* @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, isServer: boolean): boolean {
function shouldAddSentryToEntryPoint(
entryPointName: string,
isServer: boolean,
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 (isServer) {
const entryPointRoute = entryPointName.replace(/^pages/, '');
if (
// User-specified pages to skip. (Note: For ease of use, `excludeServerRoutes` is specified in terms of routes,
// which don't have the `pages` prefix.)
stringMatchesSomePattern(entryPointRoute, excludeServerRoutes, true) ||
// 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
Expand Down Expand Up @@ -462,7 +485,8 @@ export function getWebpackPluginOptions(
configFile: hasSentryProperties ? 'sentry.properties' : undefined,
stripPrefix: ['webpack://_N_E/'],
urlPrefix,
entries: (entryPointName: string) => shouldAddSentryToEntryPoint(entryPointName, isServer),
entries: (entryPointName: string) =>
shouldAddSentryToEntryPoint(entryPointName, isServer, userSentryOptions.excludeServerRoutes),
release: getSentryRelease(buildId),
dryRun: isDev,
});
Expand Down
25 changes: 25 additions & 0 deletions packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts
Expand Up @@ -241,5 +241,30 @@ 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]),
},
}),
);
});
});
});
1 change: 1 addition & 0 deletions packages/nextjs/test/integration/next.config.js
Expand Up @@ -9,6 +9,7 @@ const moduleExports = {
// Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts`
// TODO (v8): This can come out in v8, because this option will get a default value
hideSourceMaps: false,
excludeServerRoutes: ['/api/excludedEndpoints/excludedWithString', /\/api\/excludedEndpoints\/excludedWithRegExp/],
},
};
const SentryWebpackPluginOptions = {
Expand Down
4 changes: 4 additions & 0 deletions packages/nextjs/test/integration/next10.config.template
Expand Up @@ -10,6 +10,10 @@ const moduleExports = {
// Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts`
// TODO (v8): This can come out in v8, because this option will get a default value
hideSourceMaps: false,
excludeServerRoutes: [
'/api/excludedEndpoints/excludedWithString',
/\/api\/excludedEndpoints\/excludedWithRegExp/,
],
},
};

Expand Down
4 changes: 4 additions & 0 deletions packages/nextjs/test/integration/next11.config.template
Expand Up @@ -11,6 +11,10 @@ const moduleExports = {
// Suppress the warning message from `handleSourcemapHidingOptionWarning` in `src/config/webpack.ts`
// TODO (v8): This can come out in v8, because this option will get a default value
hideSourceMaps: false,
excludeServerRoutes: [
'/api/excludedEndpoints/excludedWithString',
/\/api\/excludedEndpoints\/excludedWithRegExp/,
],
},
};

Expand Down
@@ -0,0 +1,6 @@
// This file will test the `excludeServerRoutes` option when a route is provided as a RegExp.
const handler = async (): Promise<void> => {
throw new Error('API Error');
};

export default handler;
@@ -0,0 +1,6 @@
// This file will test the `excludeServerRoutes` option when a route is provided as a string.
const handler = async (): Promise<void> => {
throw new Error('API Error');
};

export default handler;
@@ -0,0 +1,67 @@
const assert = require('assert');

const { sleep } = require('../utils/common');
const { getAsync, interceptEventRequest, interceptTracingRequest } = require('../utils/server');

module.exports = async ({ url: urlBase, argv }) => {
const regExpUrl = `${urlBase}/api/excludedEndpoints/excludedWithRegExp`;
const stringUrl = `${urlBase}/api/excludedEndpoints/excludedWithString`;

const capturedRegExpErrorRequest = interceptEventRequest(
{
exception: {
values: [
{
type: 'Error',
value: 'API Error',
},
],
},
tags: {
runtime: 'node',
},
request: {
url: regExpUrl,
method: 'GET',
},
transaction: 'GET /api/excludedEndpoints/excludedWithRegExp',
},
argv,
'excluded API endpoint via RegExp',
);

const capturedStringErrorRequest = interceptEventRequest(
{
exception: {
values: [
{
type: 'Error',
value: 'API Error',
},
],
},
tags: {
runtime: 'node',
},
request: {
url: regExpUrl,
method: 'GET',
},
transaction: 'GET /api/excludedEndpoints/excludedWithString',
},
argv,
'excluded API endpoint via String',
);

await Promise.all([getAsync(regExpUrl), getAsync(stringUrl)]);
await sleep(250);

assert.ok(
!capturedRegExpErrorRequest.isDone(),
'Did intercept error request even though route should be excluded (RegExp)',
);
assert.ok(
!capturedStringErrorRequest.isDone(),
'Did intercept error request even though route should be excluded (String)',
);
};