From 0bd69f3b3e11f89ba649a8876e992787d279f11c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 31 Aug 2023 15:37:37 +0000 Subject: [PATCH 1/5] fix(nextjs): Don't rexport default in route handlers --- .../e2e-tests/test-applications/nextjs-app-dir/package.json | 2 +- .../nextjs/src/config/templates/routeHandlerWrapperTemplate.ts | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/package.json b/packages/e2e-tests/test-applications/nextjs-app-dir/package.json index d8701ef8df7c..40df65d23950 100644 --- a/packages/e2e-tests/test-applications/nextjs-app-dir/package.json +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/package.json @@ -19,7 +19,7 @@ "@types/node": "18.11.17", "@types/react": "18.0.26", "@types/react-dom": "18.0.9", - "next": "13.2.4", + "next": "13.2.19", "react": "18.2.0", "react-dom": "18.2.0", "typescript": "4.9.5", diff --git a/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts index 813f860d35de..f99500d2de67 100644 --- a/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts +++ b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts @@ -13,7 +13,6 @@ import type { RequestAsyncStorage } from './requestAsyncStorageShim'; declare const requestAsyncStorage: RequestAsyncStorage; declare const routeModule: { - default: unknown; GET?: (...args: unknown[]) => unknown; POST?: (...args: unknown[]) => unknown; PUT?: (...args: unknown[]) => unknown; @@ -72,4 +71,3 @@ export const OPTIONS = wrapHandler(routeModule.OPTIONS, 'OPTIONS'); // @ts-ignore See above // eslint-disable-next-line import/no-unresolved export * from '__SENTRY_WRAPPING_TARGET_FILE__'; -export default routeModule.default; From d479611c1f6eb679040a6f24820b394be086c432 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 31 Aug 2023 15:58:07 +0000 Subject: [PATCH 2/5] . --- .../e2e-tests/test-applications/nextjs-app-dir/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/e2e-tests/test-applications/nextjs-app-dir/package.json b/packages/e2e-tests/test-applications/nextjs-app-dir/package.json index 40df65d23950..7edc1429c308 100644 --- a/packages/e2e-tests/test-applications/nextjs-app-dir/package.json +++ b/packages/e2e-tests/test-applications/nextjs-app-dir/package.json @@ -19,7 +19,7 @@ "@types/node": "18.11.17", "@types/react": "18.0.26", "@types/react-dom": "18.0.9", - "next": "13.2.19", + "next": "13.4.19", "react": "18.2.0", "react-dom": "18.2.0", "typescript": "4.9.5", From 4c5c56327d79d8804597b915ea237f64f0e7ce17 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 31 Aug 2023 21:20:48 +0000 Subject: [PATCH 3/5] This is straight up cursed man. --- .../src/config/loaders/wrappingLoader.ts | 169 ++++++++++-------- .../templates/routeHandlerWrapperTemplate.ts | 16 +- .../templates/sentryInitWrapperTemplate.ts | 10 +- 3 files changed, 105 insertions(+), 90 deletions(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 4e19d27fea9b..031a272fdee7 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -3,6 +3,7 @@ import { stringMatchesSomePattern } from '@sentry/utils'; import * as chalk from 'chalk'; import * as fs from 'fs'; import * as path from 'path'; +import type { RollupBuild, RollupError } from 'rollup'; import { rollup } from 'rollup'; import type { VercelCronsConfig } from '../../common/types'; @@ -277,85 +278,101 @@ async function wrapUserCode( userModuleCode: string, userModuleSourceMap: any, ): Promise<{ code: string; map?: any }> { - const rollupBuild = await rollup({ - input: SENTRY_WRAPPER_MODULE_NAME, - - plugins: [ - // We're using a simple custom plugin that virtualizes our wrapper module and the user module, so we don't have to - // mess around with file paths and so that we can pass the original user module source map to rollup so that - // rollup gives us a bundle with correct source mapping to the original file - { - name: 'virtualize-sentry-wrapper-modules', - resolveId: id => { - if (id === SENTRY_WRAPPER_MODULE_NAME || id === WRAPPING_TARGET_MODULE_NAME) { - return id; - } else { - return null; - } - }, - load(id) { - if (id === SENTRY_WRAPPER_MODULE_NAME) { - return wrapperCode; - } else if (id === WRAPPING_TARGET_MODULE_NAME) { - return { - code: userModuleCode, - map: userModuleSourceMap, // give rollup acces to original user module source map - }; - } else { - return null; - } + const wrap = (withDefaultExport: boolean): Promise => + rollup({ + input: SENTRY_WRAPPER_MODULE_NAME, + + plugins: [ + // We're using a simple custom plugin that virtualizes our wrapper module and the user module, so we don't have to + // mess around with file paths and so that we can pass the original user module source map to rollup so that + // rollup gives us a bundle with correct source mapping to the original file + { + name: 'virtualize-sentry-wrapper-modules', + resolveId: id => { + if (id === SENTRY_WRAPPER_MODULE_NAME || id === WRAPPING_TARGET_MODULE_NAME) { + return id; + } else { + return null; + } + }, + load(id) { + if (id === SENTRY_WRAPPER_MODULE_NAME) { + return withDefaultExport ? wrapperCode : wrapperCode.replace('export { default } from', 'export {} from'); + } else if (id === WRAPPING_TARGET_MODULE_NAME) { + return { + code: userModuleCode, + map: userModuleSourceMap, // give rollup acces to original user module source map + }; + } else { + return null; + } + }, }, + + // 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({ + sourceMap: true, + strictRequires: true, // Don't hoist require statements that users may define + ignoreDynamicRequires: true, // Don't break dynamic requires and things like Webpack's `require.context` + ignore() { + // We want basically only want to use this plugin for handling the case where users export their handlers with module.exports. + // This plugin would also be able to convert any `require` into something esm compatible but webpack does that anyways so we just skip that part of the plugin. + // (Also, modifying require may break user code) + return true; + }, + }), + ], + + // We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external. + external: sourceId => sourceId !== SENTRY_WRAPPER_MODULE_NAME && sourceId !== WRAPPING_TARGET_MODULE_NAME, + + // Prevent rollup from stressing out about TS's use of global `this` when polyfilling await. (TS will polyfill if the + // user's tsconfig `target` is set to anything before `es2017`. See https://stackoverflow.com/a/72822340 and + // https://stackoverflow.com/a/60347490.) + context: 'this', + + // Rollup's path-resolution logic when handling re-exports can go wrong when wrapping pages which aren't at the root + // level of the `pages` directory. This may be a bug, as it doesn't match the behavior described in the docs, but what + // seems to happen is this: + // + // - We try to wrap `pages/xyz/userPage.js`, which contains `export { helperFunc } from '../../utils/helper'` + // - Rollup converts '../../utils/helper' into an absolute path + // - We mark the helper module as external + // - Rollup then converts it back to a relative path, but relative to `pages/` rather than `pages/xyz/`. (This is + // the part which doesn't match the docs. They say that Rollup will use the common ancestor of all modules in the + // bundle as the basis for the relative path calculation, but both our temporary file and the page being wrapped + // live in `pages/xyz/`, and they're the only two files in the bundle, so `pages/xyz/`` should be used as the + // root. Unclear why it's not.) + // - As a result of the miscalculation, our proxy module will include `export { helperFunc } from '../utils/helper'` + // rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in + // nextjs.. + // + // Setting `makeAbsoluteExternalsRelative` to `false` prevents all of the above by causing Rollup to ignore imports of + // externals entirely, with the result that their paths remain untouched (which is what we want). + makeAbsoluteExternalsRelative: false, + onwarn: (_warning, _warn) => { + // Suppress all warnings - we don't want to bother people with this output + // Might be stuff like "you have unused imports" + // _warn(_warning); // uncomment to debug }, + }); - // 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({ - sourceMap: true, - strictRequires: true, // Don't hoist require statements that users may define - ignoreDynamicRequires: true, // Don't break dynamic requires and things like Webpack's `require.context` - ignore() { - // We want basically only want to use this plugin for handling the case where users export their handlers with module.exports. - // This plugin would also be able to convert any `require` into something esm compatible but webpack does that anyways so we just skip that part of the plugin. - // (Also, modifying require may break user code) - return true; - }, - }), - ], - - // We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external. - external: sourceId => sourceId !== SENTRY_WRAPPER_MODULE_NAME && sourceId !== WRAPPING_TARGET_MODULE_NAME, - - // Prevent rollup from stressing out about TS's use of global `this` when polyfilling await. (TS will polyfill if the - // user's tsconfig `target` is set to anything before `es2017`. See https://stackoverflow.com/a/72822340 and - // https://stackoverflow.com/a/60347490.) - context: 'this', - - // Rollup's path-resolution logic when handling re-exports can go wrong when wrapping pages which aren't at the root - // level of the `pages` directory. This may be a bug, as it doesn't match the behavior described in the docs, but what - // seems to happen is this: - // - // - We try to wrap `pages/xyz/userPage.js`, which contains `export { helperFunc } from '../../utils/helper'` - // - Rollup converts '../../utils/helper' into an absolute path - // - We mark the helper module as external - // - Rollup then converts it back to a relative path, but relative to `pages/` rather than `pages/xyz/`. (This is - // the part which doesn't match the docs. They say that Rollup will use the common ancestor of all modules in the - // bundle as the basis for the relative path calculation, but both our temporary file and the page being wrapped - // live in `pages/xyz/`, and they're the only two files in the bundle, so `pages/xyz/`` should be used as the - // root. Unclear why it's not.) - // - As a result of the miscalculation, our proxy module will include `export { helperFunc } from '../utils/helper'` - // rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in - // nextjs.. - // - // Setting `makeAbsoluteExternalsRelative` to `false` prevents all of the above by causing Rollup to ignore imports of - // externals entirely, with the result that their paths remain untouched (which is what we want). - makeAbsoluteExternalsRelative: false, - - onwarn: (_warning, _warn) => { - // Suppress all warnings - we don't want to bother people with this output - // Might be stuff like "you have unused imports" - // _warn(_warning); // uncomment to debug - }, - }); + // Next.js sometimes complains if you define a default export (e.g. in route handlers in dev mode). + // This is why we want to avoid unnecessarily creating default exports, even if they're just `undefined`. + // For this reason we try to bundle/wrap the user code once including a re-export of `default`. + // If the user code didn't have a default export, rollup will throw. + // We then try bundling/wrapping agian, but without including a re-export of `default`. + let rollupBuild; + try { + rollupBuild = await wrap(true); + } catch (e) { + if ((e as RollupError)?.code === 'MISSING_EXPORT') { + rollupBuild = await wrap(false); + } else { + throw e; + } + } const finalBundle = await rollupBuild.generate({ format: 'esm', diff --git a/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts index f99500d2de67..7f0419501969 100644 --- a/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts +++ b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts @@ -58,6 +58,16 @@ function wrapHandler(handler: T, method: 'GET' | 'POST' | 'PUT' | 'PATCH' | ' }); } +// 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__'; + +// @ts-ignore This is the file we're wrapping +// eslint-disable-next-line import/no-unresolved +export { default } from '__SENTRY_WRAPPING_TARGET_FILE__'; + export const GET = wrapHandler(routeModule.GET, 'GET'); export const POST = wrapHandler(routeModule.POST, 'POST'); export const PUT = wrapHandler(routeModule.PUT, 'PUT'); @@ -65,9 +75,3 @@ export const PATCH = wrapHandler(routeModule.PATCH, 'PATCH'); export const DELETE = wrapHandler(routeModule.DELETE, 'DELETE'); export const HEAD = wrapHandler(routeModule.HEAD, 'HEAD'); export const OPTIONS = wrapHandler(routeModule.OPTIONS, 'OPTIONS'); - -// 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__'; diff --git a/packages/nextjs/src/config/templates/sentryInitWrapperTemplate.ts b/packages/nextjs/src/config/templates/sentryInitWrapperTemplate.ts index ab38854f090f..1720c3b62672 100644 --- a/packages/nextjs/src/config/templates/sentryInitWrapperTemplate.ts +++ b/packages/nextjs/src/config/templates/sentryInitWrapperTemplate.ts @@ -4,14 +4,8 @@ import '__SENTRY_CONFIG_IMPORT_PATH__'; // @ts-ignore This is the file we're wrapping // eslint-disable-next-line import/no-unresolved -import * as wrappee from '__SENTRY_WRAPPING_TARGET_FILE__'; - -// @ts-ignore default either exists, or it doesn't - we don't care -// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -const defaultExport = wrappee.default; +export * from '__SENTRY_WRAPPING_TARGET_FILE__'; // @ts-ignore This is the file we're wrapping // eslint-disable-next-line import/no-unresolved -export * from '__SENTRY_WRAPPING_TARGET_FILE__'; - -export default defaultExport; +export { default } from '__SENTRY_WRAPPING_TARGET_FILE__'; From 37339acf0f4ff7f2502bdb0bb1493c1d65cf60af Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 5 Sep 2023 10:05:57 +0200 Subject: [PATCH 4/5] Update packages/nextjs/src/config/loaders/wrappingLoader.ts Co-authored-by: Lukas Stracke --- 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 031a272fdee7..fc3ced90f384 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -316,7 +316,7 @@ async function wrapUserCode( strictRequires: true, // Don't hoist require statements that users may define ignoreDynamicRequires: true, // Don't break dynamic requires and things like Webpack's `require.context` ignore() { - // We want basically only want to use this plugin for handling the case where users export their handlers with module.exports. + // We basically only want to use this plugin for handling the case where users export their handlers with module.exports. // This plugin would also be able to convert any `require` into something esm compatible but webpack does that anyways so we just skip that part of the plugin. // (Also, modifying require may break user code) return true; From 5913e4c4f537a1e4f73873e5f6b498ab06c2e77a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 5 Sep 2023 08:16:19 +0000 Subject: [PATCH 5/5] rm comment --- .../nextjs/src/config/templates/routeHandlerWrapperTemplate.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts index 7f0419501969..82fc8b1ad67a 100644 --- a/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts +++ b/packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts @@ -58,8 +58,6 @@ function wrapHandler(handler: T, method: 'GET' | 'POST' | 'PUT' | 'PATCH' | ' }); } -// 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__';