-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nextjs): Add excludedServersideEntrypoints
option
#6125
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
Changes from 5 commits
09dc8bd
dbf7728
480198b
509387f
a03a09e
63b713c
21d70c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,6 +8,7 @@ import { LoaderThis } from './types'; | |||||
type LoaderOptions = { | ||||||
pagesDir: string; | ||||||
pageExtensionRegex: string; | ||||||
excludedServersideEntrypoints: (RegExp | string)[]; | ||||||
}; | ||||||
|
||||||
/** | ||||||
|
@@ -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 | ||||||
|
@@ -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. | ||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
.split(path.sep) | ||||||
.join(path.posix.sep) | ||||||
// Pull off the file extension | ||||||
.replace(new RegExp(`\\.(${pageExtensionRegex})`), ''); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: do we need to worry about windows paths here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes and that should be taken care of by this implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.") There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -91,7 +91,11 @@ export function constructWebpackConfigFunction( | |||||
use: [ | ||||||
{ | ||||||
loader: path.resolve(__dirname, 'loaders/proxyLoader.js'), | ||||||
options: { pagesDir, pageExtensionRegex }, | ||||||
options: { | ||||||
pagesDir, | ||||||
pageExtensionRegex, | ||||||
excludedServersideEntrypoints: userSentryOptions.excludedServersideEntrypoints ?? [], | ||||||
}, | ||||||
}, | ||||||
], | ||||||
}); | ||||||
|
@@ -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)) { | ||||||
|
@@ -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 | ||||||
|
@@ -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); | ||||||
} | ||||||
} | ||||||
|
@@ -377,13 +382,27 @@ 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 { | ||||||
function shouldAddSentryToEntryPoint( | ||||||
entryPointName: string, | ||||||
isServer: boolean, | ||||||
excludedServersideEntrypoints: (string | RegExp)[], | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. L: If we set a default value here, we don't have to make sure to include the
Suggested change
|
||||||
): boolean { | ||||||
const isServerSideExcluded = excludedServersideEntrypoints.some(serverSideExclude => { | ||||||
if (typeof serverSideExclude === 'string') { | ||||||
return entryPointName === serverSideExclude; | ||||||
} else { | ||||||
return entryPointName.match(serverSideExclude); | ||||||
} | ||||||
}); | ||||||
|
||||||
return ( | ||||||
entryPointName === 'pages/_app' || | ||||||
(entryPointName.includes('pages/api') && !entryPointName.includes('_middleware')) || | ||||||
(isServer && entryPointName === 'pages/_error') | ||||||
(!isServer || !isServerSideExcluded) && | ||||||
(entryPointName === 'pages/_app' || | ||||||
(entryPointName.includes('pages/api') && !entryPointName.includes('_middleware')) || | ||||||
(isServer && entryPointName === 'pages/_error')) | ||||||
lobsterkatie marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
); | ||||||
} | ||||||
|
||||||
|
@@ -436,7 +455,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, | ||||||
}); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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)', | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.