From 7e5765a48a70e0873d8c8e9e719f67c16164778e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Sun, 8 Jan 2023 12:25:48 +0000 Subject: [PATCH 1/8] ref(nextjs): Use bundling instead of proxying to wrap pages and API routes --- packages/nextjs/package.json | 2 +- packages/nextjs/rollup.npm.config.js | 4 +- .../nextjs/src/config/loaders/proxyLoader.ts | 37 +---- packages/nextjs/src/config/loaders/rollup.ts | 148 ++++++++---------- .../templates/apiProxyLoaderTemplate.ts | 4 +- .../templates/pageProxyLoaderTemplate.ts | 4 +- packages/nextjs/src/config/webpack.ts | 3 +- yarn.lock | 32 +++- 8 files changed, 107 insertions(+), 127 deletions(-) diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index 453cc7327238..86778341eb79 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -17,7 +17,7 @@ "access": "public" }, "dependencies": { - "@rollup/plugin-sucrase": "4.0.4", + "@rollup/plugin-commonjs": "24.0.0", "@rollup/plugin-virtual": "3.0.0", "@sentry/core": "7.29.0", "@sentry/integrations": "7.29.0", diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index f9bdfbf1bcbc..6827a9beb9a3 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -32,7 +32,7 @@ export default [ // make it so Rollup calms down about the fact that we're combining default and named exports exports: 'named', }, - external: ['@sentry/nextjs', '__RESOURCE_PATH__'], + external: ['@sentry/nextjs', '__SENTRY_WRAPEE__.cjs'], }, }), ), @@ -50,7 +50,7 @@ export default [ // make it so Rollup calms down about the fact that we're combining default and named exports exports: 'named', }, - external: ['@rollup/plugin-sucrase', 'rollup'], + external: ['@rollup/plugin-commonjs', '@rollup/plugin-virtual', 'rollup'], }, }), ), diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index c6bee0ec9f3a..67a21f11d095 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -1,4 +1,4 @@ -import { escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils'; +import { stringMatchesSomePattern } from '@sentry/utils'; import * as fs from 'fs'; import * as path from 'path'; @@ -44,48 +44,27 @@ export default async function proxyLoader(this: LoaderThis, userC return userCode; } - // We don't want to wrap twice (or infinitely), so in the proxy we add this query string onto references to the - // wrapped file, so that we know that it's already been processed. (Adding this query string is also necessary to - // convince webpack that it's a different file than the one it's in the middle of loading now, so that the originals - // themselves will have a chance to load.) - if (this.resourceQuery.includes('__sentry_wrapped__')) { - return userCode; - } - const templateFile = parameterizedRoute.startsWith('/api') ? 'apiProxyLoaderTemplate.js' : 'pageProxyLoaderTemplate.js'; const templatePath = path.resolve(__dirname, `../templates/${templateFile}`); - let templateCode = fs.readFileSync(templatePath).toString(); + let templateCode = fs.readFileSync(templatePath, { encoding: 'utf8' }); + // Make sure the template is included when running `webpack watch` this.addDependency(templatePath); // Inject the route and the path to the file we're wrapping into the template templateCode = templateCode.replace(/__ROUTE__/g, parameterizedRoute.replace(/\\/g, '\\\\')); - templateCode = templateCode.replace(/__RESOURCE_PATH__/g, this.resourcePath.replace(/\\/g, '\\\\')); // Run the proxy module code through Rollup, in order to split the `export * from ''` out into // individual exports (which nextjs seems to require). - let proxyCode; try { - proxyCode = await rollupize(templateCode, this.resourcePath); + return await rollupize(templateCode, userCode); } catch (err) { - __DEBUG_BUILD__ && - logger.warn( - `Could not wrap ${this.resourcePath}. An error occurred while processing the proxy module template:\n${err}`, - ); + // eslint-disable-next-line no-console + console.warn( + `[@sentry/nextjs] Could not instrument ${this.resourcePath}. An error occurred while auto-wrapping:\n${err}`, + ); return userCode; } - - // Add a query string onto all references to the wrapped file, so that webpack will consider it different from the - // non-query-stringged version (which we're already in the middle of loading as we speak), and load it separately from - // this. When the second load happens this loader will run again, but we'll be able to see the query string and will - // know to immediately return without processing. This avoids an infinite loop. - const resourceFilename = path.basename(this.resourcePath); - proxyCode = proxyCode.replace( - new RegExp(`/${escapeStringForRegex(resourceFilename)}'`, 'g'), - `/${resourceFilename}?__sentry_wrapped__'`, - ); - - return proxyCode; } diff --git a/packages/nextjs/src/config/loaders/rollup.ts b/packages/nextjs/src/config/loaders/rollup.ts index 6c7861827b95..e1a9eacd4258 100644 --- a/packages/nextjs/src/config/loaders/rollup.ts +++ b/packages/nextjs/src/config/loaders/rollup.ts @@ -1,104 +1,78 @@ -import sucrase from '@rollup/plugin-sucrase'; +import commonjs from '@rollup/plugin-commonjs'; import virtual from '@rollup/plugin-virtual'; -import { escapeStringForRegex } from '@sentry/utils'; -import * as path from 'path'; -import type { InputOptions as RollupInputOptions, OutputOptions as RollupOutputOptions } from 'rollup'; import { rollup } from 'rollup'; -const SENTRY_PROXY_MODULE_NAME = 'sentry-proxy-module'; - -const getRollupInputOptions = (templateCode: string, userModulePath: string): RollupInputOptions => ({ - input: SENTRY_PROXY_MODULE_NAME, - - plugins: [ - virtual({ - [SENTRY_PROXY_MODULE_NAME]: templateCode, - }), - - sucrase({ - transforms: ['jsx', 'typescript'], - }), - ], - - // We want to process as few files as possible, so as not to slow down the build any more than we have to. We need the - // proxy module (living in the temporary file we've created) and the file we're wrapping not to be external, because - // otherwise they won't be processed. (We need Rollup to process the former so that we can use the code, and we need - // it to process the latter so it knows what exports to re-export from the proxy module.) Past that, we don't care, so - // don't bother to process anything else. - external: importPath => importPath !== SENTRY_PROXY_MODULE_NAME && importPath !== userModulePath, - - // 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, -}); - -const rollupOutputOptions: RollupOutputOptions = { - format: 'esm', - - // Don't create a bundle - we just want the transformed entrypoint file - preserveModules: true, -}; +const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; +const WRAPEE_MODULE_NAME = '__SENTRY_WRAPEE__.cjs'; /** * Use Rollup to process the proxy module code, in order to split its `export * from ''` call into * individual exports (which nextjs seems to need). * - * Note: Any errors which occur are handled by the proxy loader which calls this function. + * Note: This function may throw in case something goes wrong while bundling. * - * @param templateCode The proxy module code - * @param userModulePath The path to the file being wrapped + * @param templateCode The wrapper module code + * @param userModuleCode The path to the file being wrapped * @returns The processed proxy module code */ -export async function rollupize(templateCode: string, userModulePath: string): Promise { - const intermediateBundle = await rollup(getRollupInputOptions(templateCode, userModulePath)); - const finalBundle = await intermediateBundle.generate(rollupOutputOptions); +export async function rollupize(templateCode: string, userModuleCode: string): Promise { + const rollupBuild = await rollup({ + input: SENTRY_WRAPPER_MODULE_NAME, - // The module at index 0 is always the entrypoint, which in this case is the proxy module. - let { code } = finalBundle.output[0]; + plugins: [ + // We're using virtual modules so we don't have to mess around with file paths + virtual({ + [SENTRY_WRAPPER_MODULE_NAME]: templateCode, + [WRAPEE_MODULE_NAME]: userModuleCode, + }), + + // 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(), + ], + + // We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external. + external: source => source !== SENTRY_WRAPPER_MODULE_NAME && source !== WRAPEE_MODULE_NAME, - // In addition to doing the desired work, Rollup also does a few things we *don't* want. Specifically, in messes up - // the path in both `import * as origModule from ''` and `export * from ''`. - // - // - It turns the square brackets surrounding each parameterized path segment into underscores. - // - It always adds `.js` to the end of the filename. - // - It converts the path from aboslute to relative, which would be fine except that when used with the virual plugin, - // it uses an incorrect (and not entirely predicable) base for that relative path. - // - // To fix this, we overwrite the messed up path with what we know it should be: `./`. (We can - // find the value of the messed up path by looking at what `import * as origModule from ''` becomes. - // Because it's the first line of the template, it's also the first line of the result, and is therefore easy to - // find.) + // 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', - const importStarStatement = code.split('\n')[0]; - // This regex should always match (we control both the input and the process which generates it, so we can guarantee - // the outcome of that processing), but just in case it somehow doesn't, we need it to throw an error so that the - // proxy loader will know to return the user's code untouched rather than returning proxy module code including a - // broken path. The non-null assertion asserts that a match has indeed been found. - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const messedUpPath = /^import \* as .* from '(.*)';$/.exec(importStarStatement)![1]; + // 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, - code = code.replace(new RegExp(escapeStringForRegex(messedUpPath), 'g'), `./${path.basename(userModulePath)}`); + onwarn: () => { + // Suppress all warnings - we don't want to bother people with this output + }, - return code; + output: { + interop: 'auto', + exports: 'named', + }, + }); + + const finalBundle = await rollupBuild.generate({ + format: 'esm', + }); + + // The module at index 0 is always the entrypoint, which in this case is the proxy module. + return finalBundle.output[0].code; } diff --git a/packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts b/packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts index 230eb55e457f..1f8ec2a3e412 100644 --- a/packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts +++ b/packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts @@ -8,7 +8,7 @@ // @ts-ignore See above // eslint-disable-next-line import/no-unresolved -import * as origModule from '__RESOURCE_PATH__'; +import * as origModule from '__SENTRY_WRAPEE__.cjs'; import * as Sentry from '@sentry/nextjs'; import type { PageConfig } from 'next'; @@ -73,4 +73,4 @@ export default exportedHandler; // 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 '__RESOURCE_PATH__'; +export * from '__SENTRY_WRAPEE__.cjs'; diff --git a/packages/nextjs/src/config/templates/pageProxyLoaderTemplate.ts b/packages/nextjs/src/config/templates/pageProxyLoaderTemplate.ts index 9979ae814c49..2c09d8c4479c 100644 --- a/packages/nextjs/src/config/templates/pageProxyLoaderTemplate.ts +++ b/packages/nextjs/src/config/templates/pageProxyLoaderTemplate.ts @@ -8,7 +8,7 @@ // @ts-ignore See above // eslint-disable-next-line import/no-unresolved -import * as wrapee from '__RESOURCE_PATH__'; +import * as wrapee from '__SENTRY_WRAPEE__.cjs'; import * as Sentry from '@sentry/nextjs'; import type { GetServerSideProps, GetStaticProps, NextPage as NextPageComponent } from 'next'; @@ -53,4 +53,4 @@ export default pageComponent; // 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 '__RESOURCE_PATH__'; +export * from '__SENTRY_WRAPEE__.cjs'; diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 988c6cc8b4b3..4e64caa2966d 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -93,7 +93,8 @@ export function constructWebpackConfigFunction( const pageExtensions = userNextConfig.pageExtensions || ['tsx', 'ts', 'jsx', 'js']; const pageExtensionRegex = pageExtensions.map(escapeStringForRegex).join('|'); - newConfig.module.rules.push({ + // 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. + newConfig.module.rules.unshift({ test: new RegExp(`^${escapeStringForRegex(pagesDir)}.*\\.(${pageExtensionRegex})$`), use: [ { diff --git a/yarn.lock b/yarn.lock index d1bfebbebeba..7f4faf2247a1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3385,6 +3385,18 @@ dependencies: web-streams-polyfill "^3.1.1" +"@rollup/plugin-commonjs@24.0.0": + version "24.0.0" + resolved "https://registry.yarnpkg.com/@rollup/plugin-commonjs/-/plugin-commonjs-24.0.0.tgz#fb7cf4a6029f07ec42b25daa535c75b05a43f75c" + integrity sha512-0w0wyykzdyRRPHOb0cQt14mIBLujfAv6GgP6g8nvg/iBxEm112t3YPPq+Buqe2+imvElTka+bjNlJ/gB56TD8g== + dependencies: + "@rollup/pluginutils" "^5.0.1" + commondir "^1.0.1" + estree-walker "^2.0.2" + glob "^8.0.3" + is-reference "1.2.1" + magic-string "^0.27.0" + "@rollup/plugin-commonjs@^15.0.0": version "15.1.0" resolved "https://registry.yarnpkg.com/@rollup/plugin-commonjs/-/plugin-commonjs-15.1.0.tgz#1e7d076c4f1b2abf7e65248570e555defc37c238" @@ -3450,7 +3462,7 @@ "@rollup/pluginutils" "^3.1.0" magic-string "^0.25.7" -"@rollup/plugin-sucrase@4.0.4", "@rollup/plugin-sucrase@^4.0.3": +"@rollup/plugin-sucrase@^4.0.3": version "4.0.4" resolved "https://registry.yarnpkg.com/@rollup/plugin-sucrase/-/plugin-sucrase-4.0.4.tgz#0a3b3d97cdc239ec3399f5a10711f751e9f95d98" integrity sha512-YH4J8yoJb5EVnLhAwWxYAQNh2SJOR+SdZ6XdgoKEv6Kxm33riYkM8MlMaggN87UoISP52qAFyZ5ey56wu6umGg== @@ -3488,6 +3500,15 @@ estree-walker "^2.0.1" picomatch "^2.2.2" +"@rollup/pluginutils@^5.0.1": + version "5.0.2" + resolved "https://registry.yarnpkg.com/@rollup/pluginutils/-/pluginutils-5.0.2.tgz#012b8f53c71e4f6f9cb317e311df1404f56e7a33" + integrity sha512-pTd9rIsP92h+B6wWwFbW8RkZv4hiR/xKsqre4SIuAOaOEQRxi0lqLke9k2/7WegC85GgUs9pjmOjCUi3In4vwA== + dependencies: + "@types/estree" "^1.0.0" + estree-walker "^2.0.2" + picomatch "^2.3.1" + "@schematics/angular@10.2.4": version "10.2.4" resolved "https://registry.yarnpkg.com/@schematics/angular/-/angular-10.2.4.tgz#3b99b9da572b57381d221e2008804e6bb9c98b82" @@ -4081,6 +4102,11 @@ resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.39.tgz#e177e699ee1b8c22d23174caaa7422644389509f" integrity sha512-EYNwp3bU+98cpU4lAWYYL7Zz+2gryWH1qbdDTidVd6hkiR6weksdbMadyXKXNPEkQFhXM+hVO9ZygomHXp+AIw== +"@types/estree@^1.0.0": + version "1.0.0" + resolved "https://registry.yarnpkg.com/@types/estree/-/estree-1.0.0.tgz#5fb2e536c1ae9bf35366eed879e827fa59ca41c2" + integrity sha512-WulqXMDUTYAXCjZnk6JtIHPigp55cVtDgDrO2gHRwhyJto21+1zbVCtOYB2L1F9w4qCQ0rOGWBnBe0FNTiEJIQ== + "@types/express-serve-static-core@4.17.28", "@types/express-serve-static-core@4.17.30", "@types/express-serve-static-core@^4.17.18": version "4.17.30" resolved "https://registry.yarnpkg.com/@types/express-serve-static-core/-/express-serve-static-core-4.17.30.tgz#0f2f99617fa8f9696170c46152ccf7500b34ac04" @@ -12742,7 +12768,7 @@ glob@7.2.0: once "^1.3.0" path-is-absolute "^1.0.0" -glob@8.0.3: +glob@8.0.3, glob@^8.0.3: version "8.0.3" resolved "https://registry.yarnpkg.com/glob/-/glob-8.0.3.tgz#415c6eb2deed9e502c68fa44a272e6da6eeca42e" integrity sha512-ull455NHSHI/Y1FqGaaYFaLGkNMMJbavMrEGFXG/PGrg6y7sutWHUHrz6gy6WEBH6akM1M414dWKCNs+IhKdiQ== @@ -14359,7 +14385,7 @@ is-potential-custom-element-name@^1.0.1: resolved "https://registry.yarnpkg.com/is-potential-custom-element-name/-/is-potential-custom-element-name-1.0.1.tgz#171ed6f19e3ac554394edf78caa05784a45bebb5" integrity sha512-bCYeRA2rVibKZd+s2625gGnGF/t7DSqDs4dP7CrLA1m7jKWz6pps0LpYLJN8Q64HtmPKJ1hrN3nzPNKFEKOUiQ== -is-reference@^1.2.1: +is-reference@1.2.1, is-reference@^1.2.1: version "1.2.1" resolved "https://registry.yarnpkg.com/is-reference/-/is-reference-1.2.1.tgz#8b2dac0b371f4bc994fdeaba9eb542d03002d0b7" integrity sha512-U82MsXXiFIrjCK4otLT+o2NA2Cd2g5MLoOVXUZjIOhLurrRxpEXzI8O0KZHr3IjLvlAH1kTPYSuqer5T9ZVBKQ== From a91da1660f0b6da860dcba8a49f69fd62bc7eb48 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Sun, 8 Jan 2023 12:31:32 +0000 Subject: [PATCH 2/8] Rename templates --- packages/nextjs/rollup.npm.config.js | 5 +---- packages/nextjs/src/config/loaders/proxyLoader.ts | 4 +--- .../{apiProxyLoaderTemplate.ts => apiWrapperTemplate.ts} | 0 .../{pageProxyLoaderTemplate.ts => pageWrapperTemplate.ts} | 0 4 files changed, 2 insertions(+), 7 deletions(-) rename packages/nextjs/src/config/templates/{apiProxyLoaderTemplate.ts => apiWrapperTemplate.ts} (100%) rename packages/nextjs/src/config/templates/{pageProxyLoaderTemplate.ts => pageWrapperTemplate.ts} (100%) diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index 6827a9beb9a3..c3da69daf595 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -14,10 +14,7 @@ export default [ ), ...makeNPMConfigVariants( makeBaseNPMConfig({ - entrypoints: [ - 'src/config/templates/pageProxyLoaderTemplate.ts', - 'src/config/templates/apiProxyLoaderTemplate.ts', - ], + entrypoints: ['src/config/templates/pageWrapperTemplate.ts', 'src/config/templates/apiWrapperTemplate.ts'], packageSpecificConfig: { output: { diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index 67a21f11d095..67844a766040 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -44,9 +44,7 @@ export default async function proxyLoader(this: LoaderThis, userC return userCode; } - const templateFile = parameterizedRoute.startsWith('/api') - ? 'apiProxyLoaderTemplate.js' - : 'pageProxyLoaderTemplate.js'; + const templateFile = parameterizedRoute.startsWith('/api') ? 'apiWrapperTemplate.js' : 'pageWrapperTemplate.js'; const templatePath = path.resolve(__dirname, `../templates/${templateFile}`); let templateCode = fs.readFileSync(templatePath, { encoding: 'utf8' }); diff --git a/packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts b/packages/nextjs/src/config/templates/apiWrapperTemplate.ts similarity index 100% rename from packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts rename to packages/nextjs/src/config/templates/apiWrapperTemplate.ts diff --git a/packages/nextjs/src/config/templates/pageProxyLoaderTemplate.ts b/packages/nextjs/src/config/templates/pageWrapperTemplate.ts similarity index 100% rename from packages/nextjs/src/config/templates/pageProxyLoaderTemplate.ts rename to packages/nextjs/src/config/templates/pageWrapperTemplate.ts From 9fb471d47150111942d14defce69bf1c60a57df9 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Sun, 8 Jan 2023 12:59:49 +0000 Subject: [PATCH 3/8] Move wrapping function into loader --- .../nextjs/src/config/loaders/proxyLoader.ts | 87 ++++++++++++++++++- packages/nextjs/src/config/loaders/rollup.ts | 78 ----------------- 2 files changed, 85 insertions(+), 80 deletions(-) delete mode 100644 packages/nextjs/src/config/loaders/rollup.ts diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index 67844a766040..e60d488c3dc2 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -1,10 +1,15 @@ +import commonjs from '@rollup/plugin-commonjs'; +import virtual from '@rollup/plugin-virtual'; import { stringMatchesSomePattern } from '@sentry/utils'; import * as fs from 'fs'; import * as path from 'path'; +import { rollup } from 'rollup'; -import { rollupize } from './rollup'; import { LoaderThis } from './types'; +const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; +const WRAPPING_TARGET_MODULE_NAME = '__SENTRY_WRAPEE__.cjs'; + type LoaderOptions = { pagesDir: string; pageExtensionRegex: string; @@ -57,7 +62,7 @@ export default async function proxyLoader(this: LoaderThis, userC // Run the proxy module code through Rollup, in order to split the `export * from ''` out into // individual exports (which nextjs seems to require). try { - return await rollupize(templateCode, userCode); + return await wrapUserCode(templateCode, userCode); } catch (err) { // eslint-disable-next-line no-console console.warn( @@ -66,3 +71,81 @@ export default async function proxyLoader(this: LoaderThis, userC return userCode; } } + +/** + * Use Rollup to process the proxy module code, in order to split its `export * from ''` call into + * individual exports (which nextjs seems to need). + * + * Wraps provided user code (located under the import defined via WRAPPING_TARGET_MODULE_NAME) with provided wrapper + * code. Under the hood, this function uses rollup to bundle the modules together. Rollup is convenient for us because + * it turns `export * from ''` (which Next.js doesn't allow) into individual named exports. + * + * Note: This function may throw in case something goes wrong while bundling. + * + * @param wrapperCode The wrapper module code + * @param userModuleCode The user module code + * @returns The wrapped user code + */ +async function wrapUserCode(wrapperCode: string, userModuleCode: string): Promise { + const rollupBuild = await rollup({ + input: SENTRY_WRAPPER_MODULE_NAME, + + plugins: [ + // We're using virtual modules so we don't have to mess around with file paths + virtual({ + [SENTRY_WRAPPER_MODULE_NAME]: wrapperCode, + [WRAPPING_TARGET_MODULE_NAME]: userModuleCode, + }), + + // 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(), + ], + + // We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external. + external: source => source !== SENTRY_WRAPPER_MODULE_NAME && source !== 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: () => { + // Suppress all warnings - we don't want to bother people with this output + }, + + output: { + // TODO + interop: 'auto', + // TODO + exports: 'named', + }, + }); + + const finalBundle = await rollupBuild.generate({ + format: 'esm', + }); + + // The module at index 0 is always the entrypoint, which in this case is the proxy module. + return finalBundle.output[0].code; +} diff --git a/packages/nextjs/src/config/loaders/rollup.ts b/packages/nextjs/src/config/loaders/rollup.ts deleted file mode 100644 index e1a9eacd4258..000000000000 --- a/packages/nextjs/src/config/loaders/rollup.ts +++ /dev/null @@ -1,78 +0,0 @@ -import commonjs from '@rollup/plugin-commonjs'; -import virtual from '@rollup/plugin-virtual'; -import { rollup } from 'rollup'; - -const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; -const WRAPEE_MODULE_NAME = '__SENTRY_WRAPEE__.cjs'; - -/** - * Use Rollup to process the proxy module code, in order to split its `export * from ''` call into - * individual exports (which nextjs seems to need). - * - * Note: This function may throw in case something goes wrong while bundling. - * - * @param templateCode The wrapper module code - * @param userModuleCode The path to the file being wrapped - * @returns The processed proxy module code - */ -export async function rollupize(templateCode: string, userModuleCode: string): Promise { - const rollupBuild = await rollup({ - input: SENTRY_WRAPPER_MODULE_NAME, - - plugins: [ - // We're using virtual modules so we don't have to mess around with file paths - virtual({ - [SENTRY_WRAPPER_MODULE_NAME]: templateCode, - [WRAPEE_MODULE_NAME]: userModuleCode, - }), - - // 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(), - ], - - // We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external. - external: source => source !== SENTRY_WRAPPER_MODULE_NAME && source !== WRAPEE_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: () => { - // Suppress all warnings - we don't want to bother people with this output - }, - - output: { - interop: 'auto', - exports: 'named', - }, - }); - - const finalBundle = await rollupBuild.generate({ - format: 'esm', - }); - - // The module at index 0 is always the entrypoint, which in this case is the proxy module. - return finalBundle.output[0].code; -} From 89f5e79d94614c9ffe74b651180f46f3f4b37a93 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Sun, 8 Jan 2023 13:05:44 +0000 Subject: [PATCH 4/8] Explain target module name --- packages/nextjs/rollup.npm.config.js | 2 +- packages/nextjs/src/config/loaders/proxyLoader.ts | 9 ++++++++- .../nextjs/src/config/templates/apiWrapperTemplate.ts | 4 ++-- .../nextjs/src/config/templates/pageWrapperTemplate.ts | 4 ++-- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index c3da69daf595..8899eac8851a 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -29,7 +29,7 @@ export default [ // make it so Rollup calms down about the fact that we're combining default and named exports exports: 'named', }, - external: ['@sentry/nextjs', '__SENTRY_WRAPEE__.cjs'], + external: ['@sentry/nextjs', '__SENTRY_WRAPPING_TARGET__'], }, }), ), diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index e60d488c3dc2..837aa6fe0de8 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -7,8 +7,12 @@ import { rollup } from 'rollup'; import { LoaderThis } from './types'; +// Just a simple placeholder to make referencing module consistent const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; -const WRAPPING_TARGET_MODULE_NAME = '__SENTRY_WRAPEE__.cjs'; + +// needs to end in .cjs in order for the `commonjs` plugin to pick it up - unfortunately the plugin has no option to +// make this work in combination with the`virtual` plugin +const WRAPPING_TARGET_MODULE_NAME = '__SENTRY_WRAPPING_TARGET__.cjs'; type LoaderOptions = { pagesDir: string; @@ -59,6 +63,9 @@ export default async function proxyLoader(this: LoaderThis, userC // 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__/g, WRAPPING_TARGET_MODULE_NAME); + // Run the proxy module code through Rollup, in order to split the `export * from ''` out into // individual exports (which nextjs seems to require). try { diff --git a/packages/nextjs/src/config/templates/apiWrapperTemplate.ts b/packages/nextjs/src/config/templates/apiWrapperTemplate.ts index 1f8ec2a3e412..db06550a3584 100644 --- a/packages/nextjs/src/config/templates/apiWrapperTemplate.ts +++ b/packages/nextjs/src/config/templates/apiWrapperTemplate.ts @@ -8,7 +8,7 @@ // @ts-ignore See above // eslint-disable-next-line import/no-unresolved -import * as origModule from '__SENTRY_WRAPEE__.cjs'; +import * as origModule from '__SENTRY_WRAPPING_TARGET__'; import * as Sentry from '@sentry/nextjs'; import type { PageConfig } from 'next'; @@ -73,4 +73,4 @@ export default exportedHandler; // 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_WRAPEE__.cjs'; +export * from '__SENTRY_WRAPPING_TARGET__'; diff --git a/packages/nextjs/src/config/templates/pageWrapperTemplate.ts b/packages/nextjs/src/config/templates/pageWrapperTemplate.ts index 2c09d8c4479c..116c0503fbef 100644 --- a/packages/nextjs/src/config/templates/pageWrapperTemplate.ts +++ b/packages/nextjs/src/config/templates/pageWrapperTemplate.ts @@ -8,7 +8,7 @@ // @ts-ignore See above // eslint-disable-next-line import/no-unresolved -import * as wrapee from '__SENTRY_WRAPEE__.cjs'; +import * as wrapee from '__SENTRY_WRAPPING_TARGET__'; import * as Sentry from '@sentry/nextjs'; import type { GetServerSideProps, GetStaticProps, NextPage as NextPageComponent } from 'next'; @@ -53,4 +53,4 @@ export default pageComponent; // 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_WRAPEE__.cjs'; +export * from '__SENTRY_WRAPPING_TARGET__'; From 0ec7960fcb7db76b13e0f5faa403e4f4f7371b00 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Sun, 8 Jan 2023 13:14:37 +0000 Subject: [PATCH 5/8] Rename proxyLoader to wrappingLoader --- packages/nextjs/src/config/loaders/index.ts | 2 +- .../config/loaders/{proxyLoader.ts => wrappingLoader.ts} | 8 ++++---- packages/nextjs/src/config/webpack.ts | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) rename packages/nextjs/src/config/loaders/{proxyLoader.ts => wrappingLoader.ts} (95%) diff --git a/packages/nextjs/src/config/loaders/index.ts b/packages/nextjs/src/config/loaders/index.ts index 150e00bf1ca4..322567c1495b 100644 --- a/packages/nextjs/src/config/loaders/index.ts +++ b/packages/nextjs/src/config/loaders/index.ts @@ -1,3 +1,3 @@ export { default as valueInjectionLoader } from './valueInjectionLoader'; export { default as prefixLoader } from './prefixLoader'; -export { default as proxyLoader } from './proxyLoader'; +export { default as wrappingLoader } from './wrappingLoader'; diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts similarity index 95% rename from packages/nextjs/src/config/loaders/proxyLoader.ts rename to packages/nextjs/src/config/loaders/wrappingLoader.ts index 837aa6fe0de8..0baa2c7a4a23 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -21,11 +21,11 @@ type LoaderOptions = { }; /** - * Replace the loaded file with a proxy module "wrapping" the original file. In the proxy, the original file is loaded, - * any data-fetching functions (`getInitialProps`, `getStaticProps`, and `getServerSideProps`) it contains are wrapped, - * and then everything is re-exported. + * Replace the loaded file with a wrapped version the original file. In the wrapped version, the original file is loaded, + * any data-fetching functions (`getInitialProps`, `getStaticProps`, and `getServerSideProps`) or API routes it contains + * are wrapped, and then everything is re-exported. */ -export default async function proxyLoader(this: LoaderThis, userCode: string): Promise { +export default async function wrappingLoader(this: LoaderThis, userCode: string): Promise { // We know one or the other will be defined, depending on the version of webpack being used const { pagesDir, diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 4e64caa2966d..3997bf3f9674 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -98,7 +98,7 @@ export function constructWebpackConfigFunction( test: new RegExp(`^${escapeStringForRegex(pagesDir)}.*\\.(${pageExtensionRegex})$`), use: [ { - loader: path.resolve(__dirname, 'loaders/proxyLoader.js'), + loader: path.resolve(__dirname, 'loaders/wrappingLoader.js'), options: { pagesDir, pageExtensionRegex, From 7af6c7b9f58e36dd88e3dd8a4dbff6566a0ffb62 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Sun, 8 Jan 2023 13:20:07 +0000 Subject: [PATCH 6/8] Extract template reading out of loader --- .../nextjs/src/config/loaders/wrappingLoader.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 0baa2c7a4a23..2e01feadff73 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -7,6 +7,12 @@ import { rollup } from 'rollup'; import { LoaderThis } from './types'; +const apiWrapperTemplatePath = path.resolve(__dirname, '..', 'templates', 'apiWrapperTemplate.js'); +const apiWrapperTemplateCode = fs.readFileSync(apiWrapperTemplatePath, { encoding: 'utf8' }); + +const pageWrapperTemplatePath = path.resolve(__dirname, '..', 'templates', 'pageWrapperTemplate.js'); +const pageWrapperTemplateCode = fs.readFileSync(pageWrapperTemplatePath, { encoding: 'utf8' }); + // Just a simple placeholder to make referencing module consistent const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; @@ -53,12 +59,7 @@ export default async function wrappingLoader(this: LoaderThis, us return userCode; } - const templateFile = parameterizedRoute.startsWith('/api') ? 'apiWrapperTemplate.js' : 'pageWrapperTemplate.js'; - const templatePath = path.resolve(__dirname, `../templates/${templateFile}`); - let templateCode = fs.readFileSync(templatePath, { encoding: 'utf8' }); - - // Make sure the template is included when running `webpack watch` - this.addDependency(templatePath); + let templateCode = parameterizedRoute.startsWith('/api') ? apiWrapperTemplateCode : pageWrapperTemplateCode; // Inject the route and the path to the file we're wrapping into the template templateCode = templateCode.replace(/__ROUTE__/g, parameterizedRoute.replace(/\\/g, '\\\\')); From 14469f65284885eced651265409af0bcfdd39d3e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Sun, 8 Jan 2023 21:46:12 +0000 Subject: [PATCH 7/8] Fix source mapping --- packages/nextjs/package.json | 1 - packages/nextjs/rollup.npm.config.js | 4 +- packages/nextjs/src/config/loaders/types.ts | 8 ++ .../src/config/loaders/wrappingLoader.ts | 97 ++++++++++++------- yarn.lock | 5 - 5 files changed, 73 insertions(+), 42 deletions(-) diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index 86778341eb79..1e6ab034d851 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -18,7 +18,6 @@ }, "dependencies": { "@rollup/plugin-commonjs": "24.0.0", - "@rollup/plugin-virtual": "3.0.0", "@sentry/core": "7.29.0", "@sentry/integrations": "7.29.0", "@sentry/node": "7.29.0", diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js index 8899eac8851a..d605dc65f3eb 100644 --- a/packages/nextjs/rollup.npm.config.js +++ b/packages/nextjs/rollup.npm.config.js @@ -36,8 +36,6 @@ export default [ ...makeNPMConfigVariants( makeBaseNPMConfig({ entrypoints: ['src/config/loaders/index.ts'], - // Needed in order to successfully import sucrase - esModuleInterop: true, packageSpecificConfig: { output: { @@ -47,7 +45,7 @@ export default [ // make it so Rollup calms down about the fact that we're combining default and named exports exports: 'named', }, - external: ['@rollup/plugin-commonjs', '@rollup/plugin-virtual', 'rollup'], + external: ['@rollup/plugin-commonjs', 'rollup'], }, }), ), diff --git a/packages/nextjs/src/config/loaders/types.ts b/packages/nextjs/src/config/loaders/types.ts index 5db902abeced..14766f077a12 100644 --- a/packages/nextjs/src/config/loaders/types.ts +++ b/packages/nextjs/src/config/loaders/types.ts @@ -1,3 +1,5 @@ +import type webpack from 'webpack'; + export type LoaderThis = { /** Path to the file being loaded */ resourcePath: string; @@ -7,6 +9,12 @@ export type LoaderThis = { // Function to add outside file used by loader to `watch` process addDependency: (filepath: string) => void; + + // Marks a loader as asynchronous + async: webpack.loader.LoaderContext['async']; + + // Return errors, code, and sourcemaps from an asynchronous loader + callback: webpack.loader.LoaderContext['callback']; } & ( | { // Loader options in Webpack 4 diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 2e01feadff73..d6f255cb9c82 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -1,11 +1,10 @@ import commonjs from '@rollup/plugin-commonjs'; -import virtual from '@rollup/plugin-virtual'; import { stringMatchesSomePattern } from '@sentry/utils'; import * as fs from 'fs'; import * as path from 'path'; import { rollup } from 'rollup'; -import { LoaderThis } from './types'; +import type { LoaderThis } from './types'; const apiWrapperTemplatePath = path.resolve(__dirname, '..', 'templates', 'apiWrapperTemplate.js'); const apiWrapperTemplateCode = fs.readFileSync(apiWrapperTemplatePath, { encoding: 'utf8' }); @@ -16,8 +15,7 @@ const pageWrapperTemplateCode = fs.readFileSync(pageWrapperTemplatePath, { encod // Just a simple placeholder to make referencing module consistent const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; -// needs to end in .cjs in order for the `commonjs` plugin to pick it up - unfortunately the plugin has no option to -// make this work in combination with the`virtual` plugin +// Needs to end in .cjs in order for the `commonjs` plugin to pick it up const WRAPPING_TARGET_MODULE_NAME = '__SENTRY_WRAPPING_TARGET__.cjs'; type LoaderOptions = { @@ -31,7 +29,13 @@ type LoaderOptions = { * any data-fetching functions (`getInitialProps`, `getStaticProps`, and `getServerSideProps`) or API routes it contains * are wrapped, and then everything is re-exported. */ -export default async function wrappingLoader(this: LoaderThis, userCode: string): Promise { +export default function wrappingLoader( + this: LoaderThis, + userCode: string, + userModuleSourceMap: any, +): void { + this.async(); + // We know one or the other will be defined, depending on the version of webpack being used const { pagesDir, @@ -56,7 +60,8 @@ export default async function wrappingLoader(this: LoaderThis, us // Skip explicitly-ignored pages if (stringMatchesSomePattern(parameterizedRoute, excludeServerRoutes, true)) { - return userCode; + this.callback(null, userCode, userModuleSourceMap); + return; } let templateCode = parameterizedRoute.startsWith('/api') ? apiWrapperTemplateCode : pageWrapperTemplateCode; @@ -69,15 +74,18 @@ export default async function wrappingLoader(this: LoaderThis, us // Run the proxy module code through Rollup, in order to split the `export * from ''` out into // individual exports (which nextjs seems to require). - try { - return await wrapUserCode(templateCode, userCode); - } catch (err) { - // eslint-disable-next-line no-console - console.warn( - `[@sentry/nextjs] Could not instrument ${this.resourcePath}. An error occurred while auto-wrapping:\n${err}`, - ); - return userCode; - } + wrapUserCode(templateCode, userCode, userModuleSourceMap) + .then(({ code: wrappedCode, map: wrappedCodeSourceMap }) => { + this.callback(null, wrappedCode, wrappedCodeSourceMap); + }) + .catch(err => { + // eslint-disable-next-line no-console + console.warn( + `[@sentry/nextjs] Could not instrument ${this.resourcePath}. An error occurred while auto-wrapping:\n${err}`, + ); + this.callback(null, userCode, userModuleSourceMap); + return; + }); } /** @@ -92,26 +100,53 @@ export default async function wrappingLoader(this: LoaderThis, us * * @param wrapperCode The wrapper module code * @param userModuleCode The user module code - * @returns The wrapped user code + * @returns The wrapped user code and a source map that describes the transformations done by this function */ -async function wrapUserCode(wrapperCode: string, userModuleCode: string): Promise { +async function wrapUserCode( + wrapperCode: string, + userModuleCode: string, + userModuleSourceMap: any, +): Promise<{ code: string; map?: any }> { const rollupBuild = await rollup({ input: SENTRY_WRAPPER_MODULE_NAME, plugins: [ - // We're using virtual modules so we don't have to mess around with file paths - virtual({ - [SENTRY_WRAPPER_MODULE_NAME]: wrapperCode, - [WRAPPING_TARGET_MODULE_NAME]: userModuleCode, - }), + // 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; + } + }, + }, // 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(), + commonjs({ + transformMixedEsModules: true, + sourceMap: true, + }), ], // We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external. - external: source => source !== SENTRY_WRAPPER_MODULE_NAME && source !== WRAPPING_TARGET_MODULE_NAME, + 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 @@ -138,22 +173,18 @@ async function wrapUserCode(wrapperCode: string, userModuleCode: string): Promis // externals entirely, with the result that their paths remain untouched (which is what we want). makeAbsoluteExternalsRelative: false, - onwarn: () => { + onwarn: (_warning, _warn) => { // Suppress all warnings - we don't want to bother people with this output - }, - - output: { - // TODO - interop: 'auto', - // TODO - exports: 'named', + // Might be stuff like "you have unused imports" + // _warn(_warning); // uncomment to debug }, }); const finalBundle = await rollupBuild.generate({ format: 'esm', + sourcemap: 'hidden', // put source map data in the bundle but don't generate a source map commment in the output }); // The module at index 0 is always the entrypoint, which in this case is the proxy module. - return finalBundle.output[0].code; + return finalBundle.output[0]; } diff --git a/yarn.lock b/yarn.lock index 7f4faf2247a1..fc5dbfc78b07 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3478,11 +3478,6 @@ "@rollup/pluginutils" "^3.1.0" resolve "^1.17.0" -"@rollup/plugin-virtual@3.0.0": - version "3.0.0" - resolved "https://registry.yarnpkg.com/@rollup/plugin-virtual/-/plugin-virtual-3.0.0.tgz#8c3f54b4ab4b267d9cd3dcbaedc58d4fd1deddca" - integrity sha512-K9KORe1myM62o0lKkNR4MmCxjwuAXsZEtIHpaILfv4kILXTOrXt/R2ha7PzMcCHPYdnkWPiBZK8ed4Zr3Ll5lQ== - "@rollup/pluginutils@^3.0.8", "@rollup/pluginutils@^3.0.9", "@rollup/pluginutils@^3.1.0": version "3.1.0" resolved "https://registry.yarnpkg.com/@rollup/pluginutils/-/pluginutils-3.1.0.tgz#706b4524ee6dc8b103b3c995533e5ad680c02b9b" From b3f23cba91695c0fdf9b41bfcc67ff148c277a31 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 10 Jan 2023 13:33:36 +0000 Subject: [PATCH 8/8] ci: Add `fail-fast: false` to playwright tests --- .github/workflows/build.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 66bf558f473a..2830b56b9eb4 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -505,6 +505,7 @@ jobs: if: needs.job_get_metadata.outputs.changed_browser_integration == 'true' || github.event_name != 'pull_request' runs-on: ubuntu-20.04 strategy: + fail-fast: false matrix: bundle: - esm