From efa8c02030931469507ab0653f461a0f86c3ebaa Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 09:22:12 +0000 Subject: [PATCH 01/21] Add simple wrapper --- packages/nextjs/rollup.npm.config.js | 2 +- packages/nextjs/src/client/index.ts | 2 ++ .../src/client/wrapAppDirComponentWithSentry.ts | 7 +++++++ packages/nextjs/src/edge/index.ts | 2 ++ .../src/edge/wrapAppDirComponentWithSentry.ts | 16 ++++++++++++++++ packages/nextjs/src/index.types.ts | 5 +++++ packages/nextjs/src/server/index.ts | 2 ++ .../src/server/wrapAppDirComponentWithSentry.tsx | 16 ++++++++++++++++ 8 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 packages/nextjs/src/client/wrapAppDirComponentWithSentry.ts create mode 100644 packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts create mode 100644 packages/nextjs/src/server/wrapAppDirComponentWithSentry.tsx diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index 9d560679b5ce..6f86d0944cfb 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -11,7 +11,7 @@ export default [ 'src/client/index.ts', 'src/server/index.ts', 'src/edge/index.ts', - 'src/config/webpack.ts', + 'src/config/index.ts', ], // prevent this internal nextjs code from ending up in our built package (this doesn't happen automatially because diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index fbd9e1eaefba..8537c15cb963 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -114,3 +114,5 @@ export { withSentryServerSideErrorGetInitialProps, wrapErrorGetInitialPropsWithSentry, } from './wrapErrorGetInitialPropsWithSentry'; + +export { wrapAppDirComponentWithSentry } from './wrapAppDirComponentWithSentry'; diff --git a/packages/nextjs/src/client/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/client/wrapAppDirComponentWithSentry.ts new file mode 100644 index 000000000000..767f2beb5be7 --- /dev/null +++ b/packages/nextjs/src/client/wrapAppDirComponentWithSentry.ts @@ -0,0 +1,7 @@ +/** + * Currently just a pass-through to provide isomorphism for the client. May be used in the future to add instrumentation + * for client components. + */ +export function wrapAppDirComponentWithSentry(wrappingTarget: any): any { + return wrappingTarget; +} diff --git a/packages/nextjs/src/edge/index.ts b/packages/nextjs/src/edge/index.ts index 712b0f61a81f..9bfd7046474f 100644 --- a/packages/nextjs/src/edge/index.ts +++ b/packages/nextjs/src/edge/index.ts @@ -139,3 +139,5 @@ export { } from './wrapApiHandlerWithSentry'; export { wrapMiddlewareWithSentry } from './wrapMiddlewareWithSentry'; + +export { wrapAppDirComponentWithSentry } from './wrapAppDirComponentWithSentry'; diff --git a/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts new file mode 100644 index 000000000000..b0816fb6b02e --- /dev/null +++ b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts @@ -0,0 +1,16 @@ +import { captureException } from '@sentry/core'; + +/** + * Wraps an `app` directory server component with Sentry error instrumentation. + */ +export function wrapAppDirComponentWithSentry(wrappingTarget: any): any { + return async function WrappedPage(this: unknown, ...args: any[]) { + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + return await wrappingTarget.apply(this, args); + } catch (e) { + captureException(e); + throw e; + } + }; +} diff --git a/packages/nextjs/src/index.types.ts b/packages/nextjs/src/index.types.ts index 28b6314c5225..99ce8bcd0556 100644 --- a/packages/nextjs/src/index.types.ts +++ b/packages/nextjs/src/index.types.ts @@ -121,3 +121,8 @@ export declare function wrapErrorGetInitialPropsWithSentry any>( getInitialProps: F, ): (...args: Parameters) => ReturnType extends Promise ? ReturnType : Promise>; + +/** + * Wraps an `app` directory component with Sentry error instrumentation. (Currently only reports errors for server components) + */ +export declare function wrapAppDirComponentWithSentry(WrappingTarget: C): C; diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts index 8ab42f9aa829..0ff8a43a0bbb 100644 --- a/packages/nextjs/src/server/index.ts +++ b/packages/nextjs/src/server/index.ts @@ -189,3 +189,5 @@ export { withSentryAPI, wrapApiHandlerWithSentry, } from './wrapApiHandlerWithSentry'; + +export { wrapAppDirComponentWithSentry } from './wrapAppDirComponentWithSentry'; diff --git a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.tsx b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.tsx new file mode 100644 index 000000000000..b0816fb6b02e --- /dev/null +++ b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.tsx @@ -0,0 +1,16 @@ +import { captureException } from '@sentry/core'; + +/** + * Wraps an `app` directory server component with Sentry error instrumentation. + */ +export function wrapAppDirComponentWithSentry(wrappingTarget: any): any { + return async function WrappedPage(this: unknown, ...args: any[]) { + try { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + return await wrappingTarget.apply(this, args); + } catch (e) { + captureException(e); + throw e; + } + }; +} From d639de8fbd95db0806e54f3690f6af8bb64e0694 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 10:33:46 +0000 Subject: [PATCH 02/21] Refactor initialization of loaders a bit --- .../src/config/loaders/wrappingLoader.ts | 72 +++++++------ packages/nextjs/src/config/webpack.ts | 100 +++++++++++++----- 2 files changed, 111 insertions(+), 61 deletions(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 3742724ae4fb..b19c7decb3c6 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -22,9 +22,11 @@ const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; const WRAPPING_TARGET_MODULE_NAME = '__SENTRY_WRAPPING_TARGET_FILE__.cjs'; type LoaderOptions = { + appDir: string; pagesDir: string; pageExtensionRegex: string; excludeServerRoutes: Array; + wrappingTargetKind: 'page' | 'api-route' | 'middleware'; }; /** @@ -42,49 +44,53 @@ export default function wrappingLoader( pagesDir, pageExtensionRegex, excludeServerRoutes = [], + wrappingTargetKind, } = 'getOptions' in this ? this.getOptions() : this.query; - this.async(); - - // Get the parameterized route name from this page's filepath - const parameterizedRoute = path - // Get the path of the file insde of the pages directory - .relative(pagesDir, this.resourcePath) - // Add a slash at the beginning - .replace(/(.*)/, '/$1') - // Pull off the file extension - .replace(new RegExp(`\\.(${pageExtensionRegex})`), '') - // Any page file named `index` corresponds to root of the directory its in, URL-wise, so turn `/xyz/index` into - // just `/xyz` - .replace(/\/index$/, '') - // In case all of the above have left us with an empty string (which will happen if we're dealing with the - // homepage), sub back in the root route - .replace(/^$/, '/'); - - // Skip explicitly-ignored pages - if (stringMatchesSomePattern(parameterizedRoute, excludeServerRoutes, true)) { - this.callback(null, userCode, userModuleSourceMap); - return; - } - - const middlewareJsPath = path.join(pagesDir, '..', 'middleware.js'); - const middlewareTsPath = path.join(pagesDir, '..', 'middleware.ts'); - let templateCode: string; - if (parameterizedRoute.startsWith('/api')) { - templateCode = apiWrapperTemplateCode; - } else if (this.resourcePath === middlewareJsPath || this.resourcePath === middlewareTsPath) { + + if (wrappingTargetKind === 'page' || wrappingTargetKind === 'api-route') { + // Get the parameterized route name from this page's filepath + const parameterizedPagesRoute = path + // Get the path of the file insde of the pages directory + .relative(pagesDir, this.resourcePath) + // Add a slash at the beginning + .replace(/(.*)/, '/$1') + // Pull off the file extension + .replace(new RegExp(`\\.(${pageExtensionRegex})`), '') + // Any page file named `index` corresponds to root of the directory its in, URL-wise, so turn `/xyz/index` into + // just `/xyz` + .replace(/\/index$/, '') + // In case all of the above have left us with an empty string (which will happen if we're dealing with the + // homepage), sub back in the root route + .replace(/^$/, '/'); + + // Skip explicitly-ignored pages + if (stringMatchesSomePattern(parameterizedPagesRoute, excludeServerRoutes, true)) { + return userCode; + } + + if (wrappingTargetKind === 'page') { + templateCode = pageWrapperTemplateCode; + } else if (wrappingTargetKind === 'api-route') { + templateCode = apiWrapperTemplateCode; + } else { + throw new Error(`Invariant: Could not get template code of unknown kind "${wrappingTargetKind}"`); + } + + // Inject the route and the path to the file we're wrapping into the template + templateCode = templateCode.replace(/__ROUTE__/g, parameterizedPagesRoute.replace(/\\/g, '\\\\')); + } else if (wrappingTargetKind === 'middleware') { templateCode = middlewareWrapperTemplateCode; } else { - templateCode = pageWrapperTemplateCode; + throw new Error(`Invariant: Could not get template code of unknown kind "${wrappingTargetKind}"`); } - // Inject the route and the path to the file we're wrapping into the template - templateCode = templateCode.replace(/__ROUTE__/g, parameterizedRoute.replace(/\\/g, '\\\\')); - // Replace the import path of the wrapping target in the template with a path that the `wrapUserCode` function will understand. templateCode = templateCode.replace(/__SENTRY_WRAPPING_TARGET_FILE__/g, WRAPPING_TARGET_MODULE_NAME); + this.async(); + // Run the proxy module code through Rollup, in order to split the `export * from ''` out into // individual exports (which nextjs seems to require). wrapUserCode(templateCode, userCode, userModuleSourceMap) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index dcb37e935ff4..16ece52b5eec 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -88,12 +88,17 @@ export function constructWebpackConfigFunction( if (isServer && userSentryOptions.autoInstrumentServerFunctions !== false) { let pagesDirPath: string; + let appDirPath: string; if (fs.existsSync(path.join(projectDir, 'pages')) && fs.lstatSync(path.join(projectDir, 'pages')).isDirectory()) { pagesDirPath = path.join(projectDir, 'pages'); + appDirPath = path.join(projectDir, 'app'); } else { pagesDirPath = path.join(projectDir, 'src', 'pages'); + appDirPath = path.join(projectDir, 'src', 'app'); } + const apiRoutesPath = path.join(pagesDirPath, 'api'); + const middlewareJsPath = path.join(pagesDirPath, '..', 'middleware.js'); const middlewareTsPath = path.join(pagesDirPath, '..', 'middleware.ts'); @@ -102,43 +107,82 @@ export function constructWebpackConfigFunction( const dotPrefixedPageExtensions = pageExtensions.map(ext => `.${ext}`); const pageExtensionRegex = pageExtensions.map(escapeStringForRegex).join('|'); - // It is very important that we insert our loader at the beginning of the array because we expect any sort of transformations/transpilations (e.g. TS -> JS) to already have happened. + const staticWrappingLoaderOptions = { + appDir: appDirPath, + pagesDir: pagesDirPath, + pageExtensionRegex, + excludeServerRoutes: userSentryOptions.excludeServerRoutes, + }; + + const normalizeLoaderResourcePath = (resourcePath: string): string => { + // `resourcePath` may be an absolute path or a path relative to the context of the webpack config + let absoluteResourcePath: string; + if (path.isAbsolute(resourcePath)) { + absoluteResourcePath = resourcePath; + } else { + absoluteResourcePath = path.join(projectDir, resourcePath); + } + + return path.normalize(absoluteResourcePath); + }; + + // It is very important that we insert our loaders at the beginning of the array because we expect any sort of transformations/transpilations (e.g. TS -> JS) to already have happened. + + // Wrap pages newConfig.module.rules.unshift({ test: resourcePath => { - // We generally want to apply the loader to all API routes, pages and to the middleware file. - - // `resourcePath` may be an absolute path or a path relative to the context of the webpack config - let absoluteResourcePath: string; - if (path.isAbsolute(resourcePath)) { - absoluteResourcePath = resourcePath; - } else { - absoluteResourcePath = path.join(projectDir, resourcePath); - } - const normalizedAbsoluteResourcePath = path.normalize(absoluteResourcePath); - - if ( - // Match everything inside pages/ with the appropriate file extension + const normalizedAbsoluteResourcePath = normalizeLoaderResourcePath(resourcePath); + return ( normalizedAbsoluteResourcePath.startsWith(pagesDirPath) && + !normalizedAbsoluteResourcePath.startsWith(apiRoutesPath) && + dotPrefixedPageExtensions.some(ext => normalizedAbsoluteResourcePath.endsWith(ext)) + ); + }, + use: [ + { + loader: path.resolve(__dirname, 'loaders', 'wrappingLoader.js'), + options: { + ...staticWrappingLoaderOptions, + wrappingTargetKind: 'page', + }, + }, + ], + }); + + // Wrap api routes + newConfig.module.rules.unshift({ + test: resourcePath => { + const normalizedAbsoluteResourcePath = normalizeLoaderResourcePath(resourcePath); + return ( + normalizedAbsoluteResourcePath.startsWith(apiRoutesPath) && dotPrefixedPageExtensions.some(ext => normalizedAbsoluteResourcePath.endsWith(ext)) - ) { - return true; - } else if ( - // Match middleware.js and middleware.ts - normalizedAbsoluteResourcePath === middlewareJsPath || - normalizedAbsoluteResourcePath === middlewareTsPath - ) { - return userSentryOptions.autoInstrumentMiddleware ?? true; - } else { - return false; - } + ); + }, + use: [ + { + loader: path.resolve(__dirname, 'loaders', 'wrappingLoader.js'), + options: { + ...staticWrappingLoaderOptions, + wrappingTargetKind: 'api-route', + }, + }, + ], + }); + + // Wrap middleware + newConfig.module.rules.unshift({ + test: resourcePath => { + const normalizedAbsoluteResourcePath = normalizeLoaderResourcePath(resourcePath); + return ( + normalizedAbsoluteResourcePath === middlewareJsPath || normalizedAbsoluteResourcePath === middlewareTsPath + ); }, use: [ { loader: path.resolve(__dirname, 'loaders', 'wrappingLoader.js'), options: { - pagesDir: pagesDirPath, - pageExtensionRegex, - excludeServerRoutes: userSentryOptions.excludeServerRoutes, + ...staticWrappingLoaderOptions, + wrappingTargetKind: 'middleware', }, }, ], From 9c24ebb98c8c815b6e6f224c17bed16b616a4d0d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 10:53:50 +0000 Subject: [PATCH 03/21] Fix build --- ...irComponentWithSentry.tsx => wrapAppDirComponentWithSentry.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename packages/nextjs/src/server/{wrapAppDirComponentWithSentry.tsx => wrapAppDirComponentWithSentry.ts} (100%) diff --git a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.tsx b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts similarity index 100% rename from packages/nextjs/src/server/wrapAppDirComponentWithSentry.tsx rename to packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts From f0f6d4ab1e7b7735e08f452f316a80ba66793d73 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 11:44:15 +0000 Subject: [PATCH 04/21] Call wrapping loader for server components --- .../src/config/loaders/wrappingLoader.ts | 52 +++++++++++--- packages/nextjs/src/config/webpack.ts | 69 ++++++++++++------- 2 files changed, 89 insertions(+), 32 deletions(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index b19c7decb3c6..a749ee93f66a 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -22,11 +22,11 @@ const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; const WRAPPING_TARGET_MODULE_NAME = '__SENTRY_WRAPPING_TARGET_FILE__.cjs'; type LoaderOptions = { - appDir: string; pagesDir: string; + appDir: string; pageExtensionRegex: string; excludeServerRoutes: Array; - wrappingTargetKind: 'page' | 'api-route' | 'middleware'; + wrappingTargetKind: 'page' | 'api-route' | 'middleware' | 'page-server-component'; }; /** @@ -38,22 +38,28 @@ export default function wrappingLoader( this: LoaderThis, userCode: string, userModuleSourceMap: any, -): void | string { +): void { // We know one or the other will be defined, depending on the version of webpack being used const { pagesDir, + appDir, pageExtensionRegex, excludeServerRoutes = [], wrappingTargetKind, } = 'getOptions' in this ? this.getOptions() : this.query; + this.async(); + let templateCode: string; if (wrappingTargetKind === 'page' || wrappingTargetKind === 'api-route') { // Get the parameterized route name from this page's filepath - const parameterizedPagesRoute = path - // Get the path of the file insde of the pages directory - .relative(pagesDir, this.resourcePath) + const parameterizedPagesRoute = path.posix + .normalize( + path + // Get the path of the file insde of the pages directory + .relative(pagesDir, this.resourcePath), + ) // Add a slash at the beginning .replace(/(.*)/, '/$1') // Pull off the file extension @@ -67,7 +73,8 @@ export default function wrappingLoader( // Skip explicitly-ignored pages if (stringMatchesSomePattern(parameterizedPagesRoute, excludeServerRoutes, true)) { - return userCode; + this.callback(null, userCode, userModuleSourceMap); + return; } if (wrappingTargetKind === 'page') { @@ -80,6 +87,35 @@ export default function wrappingLoader( // Inject the route and the path to the file we're wrapping into the template templateCode = templateCode.replace(/__ROUTE__/g, parameterizedPagesRoute.replace(/\\/g, '\\\\')); + } else if (wrappingTargetKind === 'page-server-component') { + // Get the parameterized route name from this page's filepath + const parameterizedPagesRoute = path.posix + .normalize(path.relative(appDir, this.resourcePath)) + // Add a slash at the beginning + .replace(/(.*)/, '/$1') + // Pull off the file name + .replace(/\/page\.(js|jsx|tsx)$/, '') + // Remove routing groups: https://beta.nextjs.org/docs/routing/defining-routes#example-creating-multiple-root-layouts + .replace(/\/(\(.*?\)\/)+/g, '/') + // In case all of the above have left us with an empty string (which will happen if we're dealing with the + // homepage), sub back in the root route + .replace(/^$/, '/'); + + // Skip explicitly-ignored pages + if (stringMatchesSomePattern(parameterizedPagesRoute, excludeServerRoutes, true)) { + this.callback(null, userCode, userModuleSourceMap); + return; + } + + // Skip client components - TODO: Improve this detection by scanning the AST for directives + if (userCode.match(/"use client"|'use client'/g)) { + this.callback(null, userCode, userModuleSourceMap); + return; + } + + console.log('FFF', parameterizedPagesRoute, this.resourcePath, userCode); + this.callback(null, userCode, userModuleSourceMap); + return; } else if (wrappingTargetKind === 'middleware') { templateCode = middlewareWrapperTemplateCode; } else { @@ -89,8 +125,6 @@ export default function wrappingLoader( // Replace the import path of the wrapping target in the template with a path that the `wrapUserCode` function will understand. templateCode = templateCode.replace(/__SENTRY_WRAPPING_TARGET_FILE__/g, WRAPPING_TARGET_MODULE_NAME); - this.async(); - // Run the proxy module code through Rollup, in order to split the `export * from ''` out into // individual exports (which nextjs seems to require). wrapUserCode(templateCode, userCode, userModuleSourceMap) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 16ece52b5eec..e1a6c9982369 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -86,34 +86,34 @@ export function constructWebpackConfigFunction( ], }); - if (isServer && userSentryOptions.autoInstrumentServerFunctions !== false) { - let pagesDirPath: string; - let appDirPath: string; - if (fs.existsSync(path.join(projectDir, 'pages')) && fs.lstatSync(path.join(projectDir, 'pages')).isDirectory()) { - pagesDirPath = path.join(projectDir, 'pages'); - appDirPath = path.join(projectDir, 'app'); - } else { - pagesDirPath = path.join(projectDir, 'src', 'pages'); - appDirPath = path.join(projectDir, 'src', 'app'); - } + let pagesDirPath: string; + let appDirPath: string; + if (fs.existsSync(path.join(projectDir, 'pages')) && fs.lstatSync(path.join(projectDir, 'pages')).isDirectory()) { + pagesDirPath = path.join(projectDir, 'pages'); + appDirPath = path.join(projectDir, 'app'); + } else { + pagesDirPath = path.join(projectDir, 'src', 'pages'); + appDirPath = path.join(projectDir, 'src', 'app'); + } - const apiRoutesPath = path.join(pagesDirPath, 'api'); + const apiRoutesPath = path.join(pagesDirPath, 'api'); - const middlewareJsPath = path.join(pagesDirPath, '..', 'middleware.js'); - const middlewareTsPath = path.join(pagesDirPath, '..', 'middleware.ts'); + const middlewareJsPath = path.join(pagesDirPath, '..', 'middleware.js'); + const middlewareTsPath = path.join(pagesDirPath, '..', 'middleware.ts'); - // Default page extensions per https://github.com/vercel/next.js/blob/f1dbc9260d48c7995f6c52f8fbcc65f08e627992/packages/next/server/config-shared.ts#L161 - const pageExtensions = userNextConfig.pageExtensions || ['tsx', 'ts', 'jsx', 'js']; - const dotPrefixedPageExtensions = pageExtensions.map(ext => `.${ext}`); - const pageExtensionRegex = pageExtensions.map(escapeStringForRegex).join('|'); + // Default page extensions per https://github.com/vercel/next.js/blob/f1dbc9260d48c7995f6c52f8fbcc65f08e627992/packages/next/server/config-shared.ts#L161 + const pageExtensions = userNextConfig.pageExtensions || ['tsx', 'ts', 'jsx', 'js']; + const dotPrefixedPageExtensions = pageExtensions.map(ext => `.${ext}`); + const pageExtensionRegex = pageExtensions.map(escapeStringForRegex).join('|'); - const staticWrappingLoaderOptions = { - appDir: appDirPath, - pagesDir: pagesDirPath, - pageExtensionRegex, - excludeServerRoutes: userSentryOptions.excludeServerRoutes, - }; + const staticWrappingLoaderOptions = { + appDir: appDirPath, + pagesDir: pagesDirPath, + pageExtensionRegex, + excludeServerRoutes: userSentryOptions.excludeServerRoutes, + }; + if (isServer && userSentryOptions.autoInstrumentServerFunctions !== false) { const normalizeLoaderResourcePath = (resourcePath: string): string => { // `resourcePath` may be an absolute path or a path relative to the context of the webpack config let absoluteResourcePath: string; @@ -187,6 +187,29 @@ export function constructWebpackConfigFunction( }, ], }); + + // Wrap page server components + newConfig.module.rules.unshift({ + test: resourcePath => { + const normalizedAbsoluteResourcePath = normalizeLoaderResourcePath(resourcePath); + + // ".js, .jsx, or .tsx file extensions can be used for Pages" + // https://beta.nextjs.org/docs/routing/pages-and-layouts#pages:~:text=.js%2C%20.jsx%2C%20or%20.tsx%20file%20extensions%20can%20be%20used%20for%20Pages. + return ( + normalizedAbsoluteResourcePath.startsWith(appDirPath) && + !!normalizedAbsoluteResourcePath.match(/[\\/]page\.(js|jsx|tsx)$/) + ); + }, + use: [ + { + loader: path.resolve(__dirname, 'loaders', 'wrappingLoader.js'), + options: { + ...staticWrappingLoaderOptions, + wrappingTargetKind: 'page-server-component', + }, + }, + ], + }); } // The SDK uses syntax (ES6 and ES6+ features like object spread) which isn't supported by older browsers. For users From 6b3f13a85c170ffc8e024532816738eae2f0a957 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 12:43:41 +0000 Subject: [PATCH 05/21] Auto wrap server components --- packages/nextjs/rollup.npm.config.js | 1 + .../src/config/loaders/wrappingLoader.ts | 20 +++++++---- .../serverComponentWrapperTemplate.ts | 36 +++++++++++++++++++ 3 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 packages/nextjs/src/config/templates/serverComponentWrapperTemplate.ts diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index 6f86d0944cfb..63f420f466ad 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -25,6 +25,7 @@ export default [ 'src/config/templates/pageWrapperTemplate.ts', 'src/config/templates/apiWrapperTemplate.ts', 'src/config/templates/middlewareWrapperTemplate.ts', + 'src/config/templates/serverComponentWrapperTemplate.ts', ], packageSpecificConfig: { diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index a749ee93f66a..a105e55b1ce1 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -15,6 +15,14 @@ const pageWrapperTemplateCode = fs.readFileSync(pageWrapperTemplatePath, { encod const middlewareWrapperTemplatePath = path.resolve(__dirname, '..', 'templates', 'middlewareWrapperTemplate.js'); const middlewareWrapperTemplateCode = fs.readFileSync(middlewareWrapperTemplatePath, { encoding: 'utf8' }); +const serverComponentWrapperTemplatePath = path.resolve( + __dirname, + '..', + 'templates', + 'serverComponentWrapperTemplate.js', +); +const serverComponentWrapperTemplateCode = fs.readFileSync(serverComponentWrapperTemplatePath, { encoding: 'utf8' }); + // Just a simple placeholder to make referencing module consistent const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; @@ -107,15 +115,16 @@ export default function wrappingLoader( return; } - // Skip client components - TODO: Improve this detection by scanning the AST for directives - if (userCode.match(/"use client"|'use client'/g)) { + // The following string is what Next.js injects in order to mark client components: + // https://github.com/vercel/next.js/blob/295f9da393f7d5a49b0c2e15a2f46448dbdc3895/packages/next/build/analysis/get-page-static-info.ts#L37 + // https://github.com/vercel/next.js/blob/a1c15d84d906a8adf1667332a3f0732be615afa0/packages/next-swc/crates/core/src/react_server_components.rs#L247 + // We do not want to wrap client components + if (parameterizedPagesRoute.includes('/* __next_internal_client_entry_do_not_use__ */')) { this.callback(null, userCode, userModuleSourceMap); return; } - console.log('FFF', parameterizedPagesRoute, this.resourcePath, userCode); - this.callback(null, userCode, userModuleSourceMap); - return; + templateCode = serverComponentWrapperTemplateCode; } else if (wrappingTargetKind === 'middleware') { templateCode = middlewareWrapperTemplateCode; } else { @@ -137,7 +146,6 @@ export default function wrappingLoader( `[@sentry/nextjs] Could not instrument ${this.resourcePath}. An error occurred while auto-wrapping:\n${err}`, ); this.callback(null, userCode, userModuleSourceMap); - return; }); } diff --git a/packages/nextjs/src/config/templates/serverComponentWrapperTemplate.ts b/packages/nextjs/src/config/templates/serverComponentWrapperTemplate.ts new file mode 100644 index 000000000000..4ba89238aad8 --- /dev/null +++ b/packages/nextjs/src/config/templates/serverComponentWrapperTemplate.ts @@ -0,0 +1,36 @@ +/* + * This file is a template for the code which will be substituted when our webpack loader handles non-API files in the + * `pages/` directory. + * + * We use `__SENTRY_WRAPPING_TARGET_FILE__` as a placeholder for the path to the file being wrapped. Because it's not a real package, + * this causes both TS and ESLint to complain, hence the pragma comments below. + */ + +// @ts-ignore See above +// eslint-disable-next-line import/no-unresolved +import * as wrapee from '__SENTRY_WRAPPING_TARGET_FILE__'; +// eslint-disable-next-line import/no-extraneous-dependencies +import * as Sentry from '@sentry/nextjs'; + +type ServerComponentModule = { + default: unknown; +}; + +const serverComponentModule = wrapee as ServerComponentModule; + +const serverComponent = serverComponentModule.default; + +let wrappedServerComponent; +if (typeof serverComponent === 'function') { + wrappedServerComponent = Sentry.wrapAppDirComponentWithSentry(serverComponent); +} else { + wrappedServerComponent = serverComponent; +} + +// Re-export anything exported by the page module we're wrapping. When processing this code, Rollup is smart enough to +// not include anything whose name matchs something we've explicitly exported above. +// @ts-ignore See above +// eslint-disable-next-line import/no-unresolved +export * from '__SENTRY_WRAPPING_TARGET_FILE__'; + +export default wrappedServerComponent; From bd4d8c8da6bc9d1f2cc23948fcc347c10984b078 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 12:45:07 +0000 Subject: [PATCH 06/21] Whoopsie --- packages/nextjs/src/config/loaders/wrappingLoader.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index a105e55b1ce1..ff2e10f2dd0a 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -119,7 +119,7 @@ export default function wrappingLoader( // https://github.com/vercel/next.js/blob/295f9da393f7d5a49b0c2e15a2f46448dbdc3895/packages/next/build/analysis/get-page-static-info.ts#L37 // https://github.com/vercel/next.js/blob/a1c15d84d906a8adf1667332a3f0732be615afa0/packages/next-swc/crates/core/src/react_server_components.rs#L247 // We do not want to wrap client components - if (parameterizedPagesRoute.includes('/* __next_internal_client_entry_do_not_use__ */')) { + if (userCode.includes('/* __next_internal_client_entry_do_not_use__ */')) { this.callback(null, userCode, userModuleSourceMap); return; } From 57b44dfda6fbce0c064bbad8c57e94ac86394362 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 12:54:33 +0000 Subject: [PATCH 07/21] =?UTF-8?q?=F0=9F=A4=AF?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts | 6 ++++-- packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts index b0816fb6b02e..af6b0fd07939 100644 --- a/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts +++ b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts @@ -4,10 +4,12 @@ import { captureException } from '@sentry/core'; * Wraps an `app` directory server component with Sentry error instrumentation. */ export function wrapAppDirComponentWithSentry(wrappingTarget: any): any { - return async function WrappedPage(this: unknown, ...args: any[]) { + // Super interesting: even though users may define server components as async functions, Next.js will turn them into + // synchronous functions and it will transform any`await`s into instances of the`use` hook. 🤯 + return function sentryWrappedServerComponent(this: unknown, ...args: any[]) { try { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - return await wrappingTarget.apply(this, args); + return wrappingTarget.apply(this, args); } catch (e) { captureException(e); throw e; diff --git a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts index b0816fb6b02e..af6b0fd07939 100644 --- a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts +++ b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts @@ -4,10 +4,12 @@ import { captureException } from '@sentry/core'; * Wraps an `app` directory server component with Sentry error instrumentation. */ export function wrapAppDirComponentWithSentry(wrappingTarget: any): any { - return async function WrappedPage(this: unknown, ...args: any[]) { + // Super interesting: even though users may define server components as async functions, Next.js will turn them into + // synchronous functions and it will transform any`await`s into instances of the`use` hook. 🤯 + return function sentryWrappedServerComponent(this: unknown, ...args: any[]) { try { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - return await wrappingTarget.apply(this, args); + return wrappingTarget.apply(this, args); } catch (e) { captureException(e); throw e; From c588f7877d08ed22a5126187500981b84c4c5087 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 14:30:13 +0000 Subject: [PATCH 08/21] Wrap under the assumption that server components may return promises and non-promises --- .../src/edge/wrapAppDirComponentWithSentry.ts | 19 ++++++++++++++++--- .../server/wrapAppDirComponentWithSentry.ts | 19 ++++++++++++++++--- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts index af6b0fd07939..f40436b2b087 100644 --- a/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts +++ b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts @@ -4,15 +4,28 @@ import { captureException } from '@sentry/core'; * Wraps an `app` directory server component with Sentry error instrumentation. */ export function wrapAppDirComponentWithSentry(wrappingTarget: any): any { - // Super interesting: even though users may define server components as async functions, Next.js will turn them into - // synchronous functions and it will transform any`await`s into instances of the`use` hook. 🤯 + // Even though users may define server components as async functions, for the client bundles + // Next.js will turn them into synchronous functionsf and it will transform any`await`s into instances of the`use` + // hook. 🤯 return function sentryWrappedServerComponent(this: unknown, ...args: any[]) { + let maybePromiseResult; + try { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - return wrappingTarget.apply(this, args); + maybePromiseResult = wrappingTarget.apply(this, args); } catch (e) { captureException(e); throw e; } + + if (typeof maybePromiseResult === 'object' && maybePromiseResult !== null && 'then' in maybePromiseResult) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + return maybePromiseResult.then(null, (e: Error) => { + captureException(e); + throw e; + }); + } else { + return maybePromiseResult; + } }; } diff --git a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts index af6b0fd07939..f40436b2b087 100644 --- a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts +++ b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts @@ -4,15 +4,28 @@ import { captureException } from '@sentry/core'; * Wraps an `app` directory server component with Sentry error instrumentation. */ export function wrapAppDirComponentWithSentry(wrappingTarget: any): any { - // Super interesting: even though users may define server components as async functions, Next.js will turn them into - // synchronous functions and it will transform any`await`s into instances of the`use` hook. 🤯 + // Even though users may define server components as async functions, for the client bundles + // Next.js will turn them into synchronous functionsf and it will transform any`await`s into instances of the`use` + // hook. 🤯 return function sentryWrappedServerComponent(this: unknown, ...args: any[]) { + let maybePromiseResult; + try { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - return wrappingTarget.apply(this, args); + maybePromiseResult = wrappingTarget.apply(this, args); } catch (e) { captureException(e); throw e; } + + if (typeof maybePromiseResult === 'object' && maybePromiseResult !== null && 'then' in maybePromiseResult) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + return maybePromiseResult.then(null, (e: Error) => { + captureException(e); + throw e; + }); + } else { + return maybePromiseResult; + } }; } From 1f398aef5888d175b46c991f795cd1ae6385f24a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 14:51:59 +0000 Subject: [PATCH 09/21] Add escape hatch --- packages/nextjs/src/config/types.ts | 7 ++++++- packages/nextjs/src/config/webpack.ts | 24 +++++++++++++----------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts index dd7a18f2f821..721b6d2ffb58 100644 --- a/packages/nextjs/src/config/types.ts +++ b/packages/nextjs/src/config/types.ts @@ -106,9 +106,14 @@ export type UserSentryOptions = { */ autoInstrumentMiddleware?: boolean; + /** + * Automatically instrument components in the `app` directory with error monitoring. Defaults to `true`. + */ + autoInstrumentAppDirectory?: boolean; + /** * Exclude certain serverside API routes or pages from being instrumented with Sentry. This option takes an array of - * strings or regular expressions. + * strings or regular expressions. This options also affects pages in the `app` directory. * * 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, diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index e1a6c9982369..8214f9733cb7 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -113,19 +113,19 @@ export function constructWebpackConfigFunction( excludeServerRoutes: userSentryOptions.excludeServerRoutes, }; - if (isServer && userSentryOptions.autoInstrumentServerFunctions !== false) { - const normalizeLoaderResourcePath = (resourcePath: string): string => { - // `resourcePath` may be an absolute path or a path relative to the context of the webpack config - let absoluteResourcePath: string; - if (path.isAbsolute(resourcePath)) { - absoluteResourcePath = resourcePath; - } else { - absoluteResourcePath = path.join(projectDir, resourcePath); - } + const normalizeLoaderResourcePath = (resourcePath: string): string => { + // `resourcePath` may be an absolute path or a path relative to the context of the webpack config + let absoluteResourcePath: string; + if (path.isAbsolute(resourcePath)) { + absoluteResourcePath = resourcePath; + } else { + absoluteResourcePath = path.join(projectDir, resourcePath); + } - return path.normalize(absoluteResourcePath); - }; + return path.normalize(absoluteResourcePath); + }; + if (isServer && userSentryOptions.autoInstrumentServerFunctions !== false) { // It is very important that we insert our loaders at the beginning of the array because we expect any sort of transformations/transpilations (e.g. TS -> JS) to already have happened. // Wrap pages @@ -187,7 +187,9 @@ export function constructWebpackConfigFunction( }, ], }); + } + if (userSentryOptions.autoInstrumentAppDirectory) { // Wrap page server components newConfig.module.rules.unshift({ test: resourcePath => { From 76246072fbccdb907858845e9c37ca634dc58dff Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 15:07:29 +0000 Subject: [PATCH 10/21] Add integration test --- .../app/throwing-servercomponent/layout.tsx | 3 ++ .../app/throwing-servercomponent/page.tsx | 6 ++++ .../server/autoWrappedServerComponent.test.ts | 28 +++++++++++++++++++ 3 files changed, 37 insertions(+) create mode 100644 packages/nextjs/test/integration/app/throwing-servercomponent/layout.tsx create mode 100644 packages/nextjs/test/integration/app/throwing-servercomponent/page.tsx create mode 100644 packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts diff --git a/packages/nextjs/test/integration/app/throwing-servercomponent/layout.tsx b/packages/nextjs/test/integration/app/throwing-servercomponent/layout.tsx new file mode 100644 index 000000000000..71beffba2865 --- /dev/null +++ b/packages/nextjs/test/integration/app/throwing-servercomponent/layout.tsx @@ -0,0 +1,3 @@ +export default function ({ children }: { children: React.ReactNode }) { + return <>{children}; +} diff --git a/packages/nextjs/test/integration/app/throwing-servercomponent/page.tsx b/packages/nextjs/test/integration/app/throwing-servercomponent/page.tsx new file mode 100644 index 000000000000..d3e41ae9ac7e --- /dev/null +++ b/packages/nextjs/test/integration/app/throwing-servercomponent/page.tsx @@ -0,0 +1,6 @@ +export default async function () { + // do some request so that next will render this component serverside for each new pageload + await fetch('http://example.com', { cache: 'no-store' }); + throw new Error('I am an Error thrown inside a server component'); + return

I am a server component!

; +} diff --git a/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts b/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts new file mode 100644 index 000000000000..cd71b4f61c85 --- /dev/null +++ b/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts @@ -0,0 +1,28 @@ +import { NextTestEnv } from './utils/helpers'; + +describe('Loading the throwing server component', () => { + it('should capture an error event via auto wrapping', async () => { + if (process.env.USE_APPDIR !== 'true') { + return; + } + + const env = await NextTestEnv.init(); + const url = `${env.url}/throwing-servercomponent`; + + const envelope = await env.getEnvelopeRequest({ + url, + envelopeType: 'event', + }); + + expect(envelope[2]).toMatchObject({ + exception: { + values: [ + { + type: 'Error', + value: 'I am an Error thrown inside a server component', + }, + ], + }, + }); + }); +}); From 2c89413f492fee3e7c74949c9058331bc95785a5 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 15:31:41 +0000 Subject: [PATCH 11/21] Wait wtf --- packages/nextjs/src/config/webpack.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 8214f9733cb7..879c545ad268 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -189,7 +189,7 @@ export function constructWebpackConfigFunction( }); } - if (userSentryOptions.autoInstrumentAppDirectory) { + if (isServer && userSentryOptions.autoInstrumentAppDirectory !== false) { // Wrap page server components newConfig.module.rules.unshift({ test: resourcePath => { From ec9f668f62f3149d09634ec7bc0b5f83c1f8def1 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 16:35:06 +0000 Subject: [PATCH 12/21] Testing the integration tests --- .../integration/test/server/autoWrappedServerComponent.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts b/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts index cd71b4f61c85..c69fce30c212 100644 --- a/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts +++ b/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts @@ -19,7 +19,7 @@ describe('Loading the throwing server component', () => { values: [ { type: 'Error', - value: 'I am an Error thrown inside a server component', + value: 'I am an Error thrown inside a server componnt', }, ], }, From fdf93009365d5628442cf69c94a5c8fa257d753b Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 16:35:30 +0000 Subject: [PATCH 13/21] Run server tests for app dir --- packages/nextjs/test/run-integration-tests.sh | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/test/run-integration-tests.sh b/packages/nextjs/test/run-integration-tests.sh index 30ecce9db907..d0e9ff98f198 100755 --- a/packages/nextjs/test/run-integration-tests.sh +++ b/packages/nextjs/test/run-integration-tests.sh @@ -147,12 +147,8 @@ for NEXTJS_VERSION in 10 11 12 13; do # we keep this updated as we run the tests, so that if it's ever non-zero, we can bail EXIT_CODE=0 - if [ "$USE_APPDIR" == true ]; then - echo "Skipping server tests for appdir" - else - echo "[nextjs@$NEXTJS_VERSION | webpack@$WEBPACK_VERSION] Running server tests with options: $args" - (cd .. && yarn test:integration:server) || EXIT_CODE=$? - fi + echo "[nextjs@$NEXTJS_VERSION | webpack@$WEBPACK_VERSION] Running server tests with options: $args" + (cd .. && yarn test:integration:server) || EXIT_CODE=$? if [ $EXIT_CODE -eq 0 ]; then echo "[nextjs@$NEXTJS_VERSION | webpack@$WEBPACK_VERSION] Server integration tests passed" From 5aa6cf579f6e465e833bcbbbcec621ff4eb78ce4 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 27 Jan 2023 16:47:42 +0000 Subject: [PATCH 14/21] Fix --- .../integration/test/server/autoWrappedServerComponent.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts b/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts index c69fce30c212..cd71b4f61c85 100644 --- a/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts +++ b/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts @@ -19,7 +19,7 @@ describe('Loading the throwing server component', () => { values: [ { type: 'Error', - value: 'I am an Error thrown inside a server componnt', + value: 'I am an Error thrown inside a server component', }, ], }, From cb0e21f079896a267e2c7ae2fe57340066ecda7a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 30 Jan 2023 15:08:22 +0000 Subject: [PATCH 15/21] fix(nextjs): Don't modify require calls --- packages/nextjs/src/config/loaders/wrappingLoader.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 3742724ae4fb..25628af3e0d7 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -153,7 +153,7 @@ async function wrapUserCode( // People may use `module.exports` in their API routes or page files. Next.js allows that and we also need to // handle that correctly so we let a plugin to take care of bundling cjs exports for us. commonjs({ - transformMixedEsModules: true, + strictRequires: true, sourceMap: true, }), ], From 82fae38eeb10dd0daaa885757457931d418fe372 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 30 Jan 2023 16:02:29 +0000 Subject: [PATCH 16/21] ... --- packages/nextjs/test/integration/next13.appdir.config.template | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/test/integration/next13.appdir.config.template b/packages/nextjs/test/integration/next13.appdir.config.template index 4b17134ee1d7..b016e385fdf2 100644 --- a/packages/nextjs/test/integration/next13.appdir.config.template +++ b/packages/nextjs/test/integration/next13.appdir.config.template @@ -5,7 +5,7 @@ const moduleExports = { ignoreDuringBuilds: true, }, experimental: { - appDir: Number(process.env.NODE_MAJOR) >= 16, // experimental.appDir requires Node v16.8.0 or later. + appDir: true, // experimental.appDir requires Node v16.8.0 or later. }, pageExtensions: ['jsx', 'js', 'tsx', 'ts', 'page.tsx'], sentry: { From e94ac4253a28bc0a343bb1b8c04c4ab5906ae1ba Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 1 Feb 2023 09:37:59 +0000 Subject: [PATCH 17/21] rm rf tests because they're broken --- .../app/throwing-servercomponent/layout.tsx | 3 -- .../app/throwing-servercomponent/page.tsx | 6 ---- .../server/autoWrappedServerComponent.test.ts | 28 ------------------- packages/nextjs/test/run-integration-tests.sh | 8 ++++-- 4 files changed, 6 insertions(+), 39 deletions(-) delete mode 100644 packages/nextjs/test/integration/app/throwing-servercomponent/layout.tsx delete mode 100644 packages/nextjs/test/integration/app/throwing-servercomponent/page.tsx delete mode 100644 packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts diff --git a/packages/nextjs/test/integration/app/throwing-servercomponent/layout.tsx b/packages/nextjs/test/integration/app/throwing-servercomponent/layout.tsx deleted file mode 100644 index 71beffba2865..000000000000 --- a/packages/nextjs/test/integration/app/throwing-servercomponent/layout.tsx +++ /dev/null @@ -1,3 +0,0 @@ -export default function ({ children }: { children: React.ReactNode }) { - return <>{children}; -} diff --git a/packages/nextjs/test/integration/app/throwing-servercomponent/page.tsx b/packages/nextjs/test/integration/app/throwing-servercomponent/page.tsx deleted file mode 100644 index d3e41ae9ac7e..000000000000 --- a/packages/nextjs/test/integration/app/throwing-servercomponent/page.tsx +++ /dev/null @@ -1,6 +0,0 @@ -export default async function () { - // do some request so that next will render this component serverside for each new pageload - await fetch('http://example.com', { cache: 'no-store' }); - throw new Error('I am an Error thrown inside a server component'); - return

I am a server component!

; -} diff --git a/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts b/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts deleted file mode 100644 index cd71b4f61c85..000000000000 --- a/packages/nextjs/test/integration/test/server/autoWrappedServerComponent.test.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { NextTestEnv } from './utils/helpers'; - -describe('Loading the throwing server component', () => { - it('should capture an error event via auto wrapping', async () => { - if (process.env.USE_APPDIR !== 'true') { - return; - } - - const env = await NextTestEnv.init(); - const url = `${env.url}/throwing-servercomponent`; - - const envelope = await env.getEnvelopeRequest({ - url, - envelopeType: 'event', - }); - - expect(envelope[2]).toMatchObject({ - exception: { - values: [ - { - type: 'Error', - value: 'I am an Error thrown inside a server component', - }, - ], - }, - }); - }); -}); diff --git a/packages/nextjs/test/run-integration-tests.sh b/packages/nextjs/test/run-integration-tests.sh index d0e9ff98f198..30ecce9db907 100755 --- a/packages/nextjs/test/run-integration-tests.sh +++ b/packages/nextjs/test/run-integration-tests.sh @@ -147,8 +147,12 @@ for NEXTJS_VERSION in 10 11 12 13; do # we keep this updated as we run the tests, so that if it's ever non-zero, we can bail EXIT_CODE=0 - echo "[nextjs@$NEXTJS_VERSION | webpack@$WEBPACK_VERSION] Running server tests with options: $args" - (cd .. && yarn test:integration:server) || EXIT_CODE=$? + if [ "$USE_APPDIR" == true ]; then + echo "Skipping server tests for appdir" + else + echo "[nextjs@$NEXTJS_VERSION | webpack@$WEBPACK_VERSION] Running server tests with options: $args" + (cd .. && yarn test:integration:server) || EXIT_CODE=$? + fi if [ $EXIT_CODE -eq 0 ]; then echo "[nextjs@$NEXTJS_VERSION | webpack@$WEBPACK_VERSION] Server integration tests passed" From 2948f0b92d519498f51a7bd05bd0b6a1c54dd192 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 1 Feb 2023 10:53:17 +0000 Subject: [PATCH 18/21] Use proxy --- .../src/edge/wrapAppDirComponentWithSentry.ts | 39 ++++++++++--------- packages/nextjs/src/index.types.ts | 2 +- .../server/wrapAppDirComponentWithSentry.ts | 39 ++++++++++--------- 3 files changed, 41 insertions(+), 39 deletions(-) diff --git a/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts index f40436b2b087..c69038165914 100644 --- a/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts +++ b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts @@ -3,29 +3,30 @@ import { captureException } from '@sentry/core'; /** * Wraps an `app` directory server component with Sentry error instrumentation. */ -export function wrapAppDirComponentWithSentry(wrappingTarget: any): any { +export function wrapAppDirComponentWithSentry any>(appDirComponent: F): F { // Even though users may define server components as async functions, for the client bundles // Next.js will turn them into synchronous functionsf and it will transform any`await`s into instances of the`use` // hook. 🤯 - return function sentryWrappedServerComponent(this: unknown, ...args: any[]) { - let maybePromiseResult; + return new Proxy(appDirComponent, { + apply: (originalFunction, thisArg, args) => { + let maybePromiseResult; - try { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - maybePromiseResult = wrappingTarget.apply(this, args); - } catch (e) { - captureException(e); - throw e; - } - - if (typeof maybePromiseResult === 'object' && maybePromiseResult !== null && 'then' in maybePromiseResult) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - return maybePromiseResult.then(null, (e: Error) => { + try { + maybePromiseResult = originalFunction.apply(thisArg, args); + } catch (e) { captureException(e); throw e; - }); - } else { - return maybePromiseResult; - } - }; + } + + if (typeof maybePromiseResult === 'object' && maybePromiseResult !== null && 'then' in maybePromiseResult) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + return maybePromiseResult.then(null, (e: Error) => { + captureException(e); + throw e; + }); + } else { + return maybePromiseResult; + } + }, + }); } diff --git a/packages/nextjs/src/index.types.ts b/packages/nextjs/src/index.types.ts index 28a13a662b95..aca9f9d48211 100644 --- a/packages/nextjs/src/index.types.ts +++ b/packages/nextjs/src/index.types.ts @@ -129,4 +129,4 @@ export declare function withSentryServerSideErrorGetInitialProps(WrappingTarget: C): C; +export declare function wrapAppDirComponentWithSentry any>(WrappingTarget: F): F; diff --git a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts index f40436b2b087..c69038165914 100644 --- a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts +++ b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts @@ -3,29 +3,30 @@ import { captureException } from '@sentry/core'; /** * Wraps an `app` directory server component with Sentry error instrumentation. */ -export function wrapAppDirComponentWithSentry(wrappingTarget: any): any { +export function wrapAppDirComponentWithSentry any>(appDirComponent: F): F { // Even though users may define server components as async functions, for the client bundles // Next.js will turn them into synchronous functionsf and it will transform any`await`s into instances of the`use` // hook. 🤯 - return function sentryWrappedServerComponent(this: unknown, ...args: any[]) { - let maybePromiseResult; + return new Proxy(appDirComponent, { + apply: (originalFunction, thisArg, args) => { + let maybePromiseResult; - try { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - maybePromiseResult = wrappingTarget.apply(this, args); - } catch (e) { - captureException(e); - throw e; - } - - if (typeof maybePromiseResult === 'object' && maybePromiseResult !== null && 'then' in maybePromiseResult) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - return maybePromiseResult.then(null, (e: Error) => { + try { + maybePromiseResult = originalFunction.apply(thisArg, args); + } catch (e) { captureException(e); throw e; - }); - } else { - return maybePromiseResult; - } - }; + } + + if (typeof maybePromiseResult === 'object' && maybePromiseResult !== null && 'then' in maybePromiseResult) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + return maybePromiseResult.then(null, (e: Error) => { + captureException(e); + throw e; + }); + } else { + return maybePromiseResult; + } + }, + }); } From 990ff58d3a358d7a2f5a990b8d74b6f43ad1af99 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 2 Feb 2023 11:45:18 +0000 Subject: [PATCH 19/21] Remove leftover --- packages/nextjs/test/integration/next13.appdir.config.template | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/test/integration/next13.appdir.config.template b/packages/nextjs/test/integration/next13.appdir.config.template index b016e385fdf2..4b17134ee1d7 100644 --- a/packages/nextjs/test/integration/next13.appdir.config.template +++ b/packages/nextjs/test/integration/next13.appdir.config.template @@ -5,7 +5,7 @@ const moduleExports = { ignoreDuringBuilds: true, }, experimental: { - appDir: true, // experimental.appDir requires Node v16.8.0 or later. + appDir: Number(process.env.NODE_MAJOR) >= 16, // experimental.appDir requires Node v16.8.0 or later. }, pageExtensions: ['jsx', 'js', 'tsx', 'ts', 'page.tsx'], sentry: { From 9401d66053a1f5ebafad2fbaf572bab33097a86f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 2 Feb 2023 12:57:17 +0000 Subject: [PATCH 20/21] Add tests --- packages/nextjs/test/config/loaders.test.ts | 146 ++++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/packages/nextjs/test/config/loaders.test.ts b/packages/nextjs/test/config/loaders.test.ts index 714d3388782c..9c56f97e5b3b 100644 --- a/packages/nextjs/test/config/loaders.test.ts +++ b/packages/nextjs/test/config/loaders.test.ts @@ -1,6 +1,7 @@ // mock helper functions not tested directly in this file import './mocks'; +import type { ModuleRuleUseProperty, WebpackModuleRule } from '../../src/config/types'; import { clientBuildContext, clientWebpackConfig, @@ -38,6 +39,29 @@ declare global { } } +function applyRuleToResource(rule: WebpackModuleRule, resourcePath: string): ModuleRuleUseProperty[] { + const applications = []; + + let shouldApply: boolean = false; + if (typeof rule.test === 'function') { + shouldApply = rule.test(resourcePath); + } else if (rule.test instanceof RegExp) { + shouldApply = !!resourcePath.match(rule.test); + } else if (rule.test) { + shouldApply = resourcePath === rule.test; + } + + if (shouldApply) { + if (Array.isArray(rule.use)) { + applications.push(...rule.use); + } else if (rule.use) { + applications.push(rule.use); + } + } + + return applications; +} + describe('webpack loaders', () => { describe('server loaders', () => { it('adds server `valueInjection` loader to server config', async () => { @@ -60,6 +84,128 @@ describe('webpack loaders', () => { ], }); }); + + it.each([ + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/pages/testPage.tsx', + expectedWrappingTargetKind: 'page', + }, + { + resourcePath: './src/pages/testPage.tsx', + expectedWrappingTargetKind: 'page', + }, + { + resourcePath: './pages/testPage.tsx', + expectedWrappingTargetKind: undefined, + }, + { + resourcePath: '../src/pages/testPage.tsx', + expectedWrappingTargetKind: undefined, + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/pages/nested/testPage.ts', + expectedWrappingTargetKind: 'page', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/pages/nested/testPage.js', + expectedWrappingTargetKind: 'page', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/pages/[nested]/[testPage].js', + expectedWrappingTargetKind: 'page', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/pages/[...testPage].js', + expectedWrappingTargetKind: 'page', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/middleware.js', + expectedWrappingTargetKind: 'middleware', + }, + { + resourcePath: './src/middleware.js', + expectedWrappingTargetKind: 'middleware', + }, + { + resourcePath: './middleware.js', + expectedWrappingTargetKind: undefined, + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/middleware.ts', + expectedWrappingTargetKind: 'middleware', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/middleware.tsx', + expectedWrappingTargetKind: undefined, + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/pages/api/testApiRoute.ts', + expectedWrappingTargetKind: 'api-route', + }, + { + resourcePath: './src/pages/api/testApiRoute.ts', + expectedWrappingTargetKind: 'api-route', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/pages/api/nested/testApiRoute.js', + expectedWrappingTargetKind: 'api-route', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/app/page.js', + expectedWrappingTargetKind: 'page-server-component', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/app/nested/page.js', + expectedWrappingTargetKind: 'page-server-component', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/app/nested/page.ts', // ts is not a valid file ending for pages in the app dir + expectedWrappingTargetKind: undefined, + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/app/(group)/nested/page.tsx', + expectedWrappingTargetKind: 'page-server-component', + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/app/(group)/nested/loading.ts', + expectedWrappingTargetKind: undefined, + }, + { + resourcePath: '/Users/Maisey/projects/squirrelChasingSimulator/src/app/layout.js', + expectedWrappingTargetKind: undefined, + }, + ])( + 'should apply the right wrappingTargetKind with wrapping loader ($resourcePath)', + async ({ resourcePath, expectedWrappingTargetKind }) => { + const finalWebpackConfig = await materializeFinalWebpackConfig({ + exportedNextConfig, + incomingWebpackConfig: serverWebpackConfig, + incomingWebpackBuildContext: serverBuildContext, + }); + + const loaderApplications: ModuleRuleUseProperty[] = []; + finalWebpackConfig.module.rules.forEach(rule => { + loaderApplications.push(...applyRuleToResource(rule, resourcePath)); + }); + + if (expectedWrappingTargetKind) { + expect(loaderApplications).toContainEqual( + expect.objectContaining({ + loader: expect.stringMatching(/wrappingLoader\.js$/), + options: expect.objectContaining({ + wrappingTargetKind: expectedWrappingTargetKind, + }), + }), + ); + } else { + expect(loaderApplications).not.toContainEqual( + expect.objectContaining({ + loader: expect.stringMatching(/wrappingLoader\.js$/), + }), + ); + } + }, + ); }); describe('client loaders', () => { From 0674a9d4bc15fdb364de51f40459b7cf9f55801c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 2 Feb 2023 13:00:23 +0000 Subject: [PATCH 21/21] Fix typo --- packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts | 2 +- packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts index c69038165914..116df75a4caa 100644 --- a/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts +++ b/packages/nextjs/src/edge/wrapAppDirComponentWithSentry.ts @@ -5,7 +5,7 @@ import { captureException } from '@sentry/core'; */ export function wrapAppDirComponentWithSentry any>(appDirComponent: F): F { // Even though users may define server components as async functions, for the client bundles - // Next.js will turn them into synchronous functionsf and it will transform any`await`s into instances of the`use` + // Next.js will turn them into synchronous functions and it will transform any`await`s into instances of the`use` // hook. 🤯 return new Proxy(appDirComponent, { apply: (originalFunction, thisArg, args) => { diff --git a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts index c69038165914..116df75a4caa 100644 --- a/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts +++ b/packages/nextjs/src/server/wrapAppDirComponentWithSentry.ts @@ -5,7 +5,7 @@ import { captureException } from '@sentry/core'; */ export function wrapAppDirComponentWithSentry any>(appDirComponent: F): F { // Even though users may define server components as async functions, for the client bundles - // Next.js will turn them into synchronous functionsf and it will transform any`await`s into instances of the`use` + // Next.js will turn them into synchronous functions and it will transform any`await`s into instances of the`use` // hook. 🤯 return new Proxy(appDirComponent, { apply: (originalFunction, thisArg, args) => {