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 excludedServersideEntrypoints option #6125

Closed
wants to merge 7 commits into from
Closed
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
23 changes: 21 additions & 2 deletions packages/nextjs/src/config/loaders/proxyLoader.ts
Expand Up @@ -8,6 +8,7 @@ import { LoaderThis } from './types';
type LoaderOptions = {
pagesDir: string;
pageExtensionRegex: string;
excludedServersideEntrypoints: (RegExp | string)[];
};

/**
Expand All @@ -17,7 +18,8 @@ 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, excludedServersideEntrypoints } =
'getOptions' in this ? this.getOptions() : this.query;

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

// For the `excludedServersideEntrypoints` option we need the calculate the relative path to the file in question without file extension.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// For the `excludedServersideEntrypoints` option we need the calculate the relative path to the file in question without file extension.
// For the `excludedServersideEntrypoints` option we need to calculate the relative path to the file in question without file extension.

const relativePosixPagePath = path
.join('pages', path.relative(pagesDir, this.resourcePath))
// Make sure that path is in posix style - this should make it easiser for users to configure and copy & paste from docs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Make sure that path is in posix style - this should make it easiser for users to configure and copy & paste from docs
// Make sure that path is in posix style (using forward slashes) - this should make it easier for users to configure and will make docs examples work on all operating systems

.split(path.sep)
.join(path.posix.sep)
// Pull off the file extension
.replace(new RegExp(`\\.(${pageExtensionRegex})`), '');
Copy link
Member

Choose a reason for hiding this comment

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

m: do we need to worry about windows paths 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.

Yes and that should be taken care of by this implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok actually went back and made sure that the path we match against is posix because it is just way easier for users to configure that way


const isExcluded = excludedServersideEntrypoints.some(exludeEntry => {
if (typeof exludeEntry === 'string') {
return relativePosixPagePath === exludeEntry;
} else {
return relativePosixPagePath.match(exludeEntry);
}
});
Comment on lines +48 to +54
Copy link
Member

Choose a reason for hiding this comment

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

L: I don't immediately have a great suggestion here, but the fact that we're comparing the excluded paths to the current path rather than vice versa makes my brain hurt a little. (When I put it into words in my head, it feels more intuitive to say "if the current path matches any of the excluded paths" rather than "if any of the excluded paths match the current path.")

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly wouldn't know how to stop the brain-hurting here :D We only have an array and a string to work with - can we even reverse this logic?


// 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
// themselves will have a chance to load.)
if (this.resourceQuery.includes('__sentry_wrapped__')) {
if (isExcluded || this.resourceQuery.includes('__sentry_wrapped__')) {
return userCode;
}

Expand Down
9 changes: 9 additions & 0 deletions packages/nextjs/src/config/types.ts
Expand Up @@ -59,6 +59,15 @@ export type UserSentryOptions = {

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

// Used to exclude certain serverside API routes or pages from being instrumented with Sentry. This option takes an
Copy link
Member

Choose a reason for hiding this comment

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

H: See above question re: if non-API pages should be part of this system.

// array of strings or regular expressions - strings will exactly match a route. Matches are made against routes in
// the following form:
// - "pages/home/index"
// - "pages/about"
// - "pages/posts/[postId]"
// - "pages/posts/[postId]/comments"
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
excludedServersideEntrypoints?: (RegExp | string)[];
};

export type NextConfigFunction = (phase: string, defaults: { defaultConfig: NextConfigObject }) => NextConfigObject;
Expand Down
51 changes: 41 additions & 10 deletions packages/nextjs/src/config/webpack.ts
Expand Up @@ -91,7 +91,11 @@ export function constructWebpackConfigFunction(
use: [
{
loader: path.resolve(__dirname, 'loaders/proxyLoader.js'),
options: { pagesDir, pageExtensionRegex },
options: {
pagesDir,
pageExtensionRegex,
excludedServersideEntrypoints: userSentryOptions.excludedServersideEntrypoints,
},
},
],
});
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,7 +273,7 @@ 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.excludedServersideEntrypoints ?? [])) {
addFilesToExistingEntryPoint(newEntryProperty, entryPointName, filesToInject);
}
}
Expand Down Expand Up @@ -377,14 +382,39 @@ 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 excludedServersideEntrypoints A list of excluded serverside entrypoints
* @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')
);
function shouldAddSentryToEntryPoint(
entryPointName: string,
isServer: boolean,
excludedServersideEntrypoints: (string | RegExp)[] = [],
): boolean {
if (isServer) {
const isExcluded = excludedServersideEntrypoints.some(serverSideExclude => {
if (typeof serverSideExclude === 'string') {
return entryPointName === serverSideExclude;
} else {
return entryPointName.match(serverSideExclude);
}
});

if (isExcluded) {
return false;
} else if (entryPointName === 'pages/_error') {
return true;
}
}

if (entryPointName === 'pages/_app') {
return true;
}

if (entryPointName.includes('pages/api') && !entryPointName.includes('_middleware')) {
return true;
}

return false;
}

/**
Expand Down Expand Up @@ -436,7 +466,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.excludedServersideEntrypoints),
release: getSentryRelease(buildId),
dryRun: isDev,
});
Expand Down
4 changes: 4 additions & 0 deletions packages/nextjs/test/integration/next.config.js
Expand Up @@ -9,6 +9,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,
excludedServersideEntrypoints: [
'pages/api/excludedEndpoints/excludedWithString',
/pages\/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,
excludedServersideEntrypoints: [
'pages/api/excludedEndpoints/excludedWithString',
/pages\/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,
excludedServersideEntrypoints: [
'pages/api/excludedEndpoints/excludedWithString',
/pages\/api\/excludedEndpoints\/excludedWithRegExp/,
],
},
};

Expand Down
@@ -0,0 +1,6 @@
// This file will test the `excludedServersideEntrypoints` 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 `excludedServersideEntrypoints` 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)',
);
lforst marked this conversation as resolved.
Show resolved Hide resolved
assert.ok(
!capturedStringErrorRequest.isDone(),
'Did intercept error request even though route should be excluded (String)',
);
};