From 53be99d4fb2e14221202a79200ae315d0fe09de7 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Sun, 23 Oct 2022 22:35:48 -0700 Subject: [PATCH 1/4] move rollup error handling to proxy loader --- .../nextjs/src/config/loaders/proxyLoader.ts | 17 +++++++++------ packages/nextjs/src/config/loaders/rollup.ts | 21 ++++++------------- 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/packages/nextjs/src/config/loaders/proxyLoader.ts b/packages/nextjs/src/config/loaders/proxyLoader.ts index 2c499211800a..e4e7e743ae53 100644 --- a/packages/nextjs/src/config/loaders/proxyLoader.ts +++ b/packages/nextjs/src/config/loaders/proxyLoader.ts @@ -1,4 +1,4 @@ -import { escapeStringForRegex } from '@sentry/utils'; +import { escapeStringForRegex, logger } from '@sentry/utils'; import * as fs from 'fs'; import * as path from 'path'; @@ -67,12 +67,17 @@ 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), then delete the tempoary file. - let proxyCode = await rollupize(tempFilePath, this.resourcePath); - fs.unlinkSync(tempFilePath); - - if (!proxyCode) { - // We will already have thrown a warning in `rollupize`, so no need to do it again here + let proxyCode; + try { + proxyCode = await rollupize(tempFilePath, this.resourcePath); + } catch (err) { + __DEBUG_BUILD__ && + logger.warn( + `Could not wrap ${this.resourcePath}. An error occurred while processing the proxy module template:\n${err}`, + ); return userCode; + } finally { + fs.unlinkSync(tempFilePath); } // Add a query string onto all references to the wrapped file, so that webpack will consider it different from the diff --git a/packages/nextjs/src/config/loaders/rollup.ts b/packages/nextjs/src/config/loaders/rollup.ts index 67daf63b0eca..5ff2dea0a2ef 100644 --- a/packages/nextjs/src/config/loaders/rollup.ts +++ b/packages/nextjs/src/config/loaders/rollup.ts @@ -1,6 +1,5 @@ import type { RollupSucraseOptions } from '@rollup/plugin-sucrase'; import sucrase from '@rollup/plugin-sucrase'; -import { logger } from '@sentry/utils'; import * as path from 'path'; import type { InputOptions as RollupInputOptions, OutputOptions as RollupOutputOptions } from 'rollup'; import { rollup } from 'rollup'; @@ -65,23 +64,15 @@ const rollupOutputOptions: RollupOutputOptions = { * Use Rollup to process the proxy module file (located at `tempProxyFilePath`) 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. + * * @param tempProxyFilePath The path to the temporary file containing the proxy module code * @param resourcePath The path to the file being wrapped - * @returns The processed proxy module code, or undefined if an error occurs + * @returns The processed proxy module code */ -export async function rollupize(tempProxyFilePath: string, resourcePath: string): Promise { - let finalBundle; - - try { - const intermediateBundle = await rollup(getRollupInputOptions(tempProxyFilePath, resourcePath)); - finalBundle = await intermediateBundle.generate(rollupOutputOptions); - } catch (err) { - __DEBUG_BUILD__ && - logger.warn( - `Could not wrap ${resourcePath}. An error occurred while processing the proxy module template:\n${err}`, - ); - return undefined; - } +export async function rollupize(tempProxyFilePath: string, resourcePath: string): Promise { + const intermediateBundle = await rollup(getRollupInputOptions(tempProxyFilePath, resourcePath)); + const finalBundle = await intermediateBundle.generate(rollupOutputOptions); // The module at index 0 is always the entrypoint, which in this case is the proxy module. let { code } = finalBundle.output[0]; From 8ea8601d46f261a0654023d02558c4a130dcbe16 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Sun, 23 Oct 2022 22:38:17 -0700 Subject: [PATCH 2/4] simplify typing of `getRollupInputOptions` --- packages/nextjs/src/config/loaders/rollup.ts | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/config/loaders/rollup.ts b/packages/nextjs/src/config/loaders/rollup.ts index 5ff2dea0a2ef..066d97598454 100644 --- a/packages/nextjs/src/config/loaders/rollup.ts +++ b/packages/nextjs/src/config/loaders/rollup.ts @@ -1,22 +1,15 @@ -import type { RollupSucraseOptions } from '@rollup/plugin-sucrase'; import sucrase from '@rollup/plugin-sucrase'; import * as path from 'path'; import type { InputOptions as RollupInputOptions, OutputOptions as RollupOutputOptions } from 'rollup'; import { rollup } from 'rollup'; -const getRollupInputOptions: (proxyPath: string, resourcePath: string) => RollupInputOptions = ( - proxyPath, - resourcePath, -) => ({ +const getRollupInputOptions = (proxyPath: string, resourcePath: string): RollupInputOptions => ({ input: proxyPath, + plugins: [ - // For some reason, even though everything in `RollupSucraseOptions` besides `transforms` is supposed to be - // optional, TS complains that there are a bunch of missing properties (hence the typecast). Similar to - // https://github.com/microsoft/TypeScript/issues/20722, though that's been fixed. (In this case it's an interface - // exporting a `Pick` picking optional properties which is turning them required somehow.)' sucrase({ transforms: ['jsx', 'typescript'], - } as unknown as RollupSucraseOptions), + }), ], // 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 From e870a2d1442a41fcb95663101247062433022340 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Sun, 23 Oct 2022 22:51:20 -0700 Subject: [PATCH 3/4] simplify comment about `makeAbsoluteExternalsRelative` --- packages/nextjs/src/config/loaders/rollup.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/config/loaders/rollup.ts b/packages/nextjs/src/config/loaders/rollup.ts index 066d97598454..0a3f9e306cdd 100644 --- a/packages/nextjs/src/config/loaders/rollup.ts +++ b/packages/nextjs/src/config/loaders/rollup.ts @@ -40,9 +40,8 @@ const getRollupInputOptions = (proxyPath: string, resourcePath: string): RollupI // rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in // nextjs.. // - // It's not 100% clear why, but telling it not to do the conversion back from absolute to relative (by setting - // `makeAbsoluteExternalsRelative` to `false`) seems to also prevent it from going from relative to absolute in the - // first place, with the result that the path remains untouched (which is what we want.) + // 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, }); From 0e7b4fd4f69caa2537951726166801b4458ca262 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Sun, 23 Oct 2022 22:54:33 -0700 Subject: [PATCH 4/4] s/resourcePath/userModulePath --- packages/nextjs/src/config/loaders/rollup.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/nextjs/src/config/loaders/rollup.ts b/packages/nextjs/src/config/loaders/rollup.ts index 0a3f9e306cdd..d04646da58a9 100644 --- a/packages/nextjs/src/config/loaders/rollup.ts +++ b/packages/nextjs/src/config/loaders/rollup.ts @@ -3,7 +3,7 @@ import * as path from 'path'; import type { InputOptions as RollupInputOptions, OutputOptions as RollupOutputOptions } from 'rollup'; import { rollup } from 'rollup'; -const getRollupInputOptions = (proxyPath: string, resourcePath: string): RollupInputOptions => ({ +const getRollupInputOptions = (proxyPath: string, userModulePath: string): RollupInputOptions => ({ input: proxyPath, plugins: [ @@ -17,7 +17,7 @@ const getRollupInputOptions = (proxyPath: string, resourcePath: string): RollupI // 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 !== proxyPath && importPath !== resourcePath, + external: importPath => importPath !== proxyPath && 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 @@ -59,11 +59,11 @@ const rollupOutputOptions: RollupOutputOptions = { * Note: Any errors which occur are handled by the proxy loader which calls this function. * * @param tempProxyFilePath The path to the temporary file containing the proxy module code - * @param resourcePath The path to the file being wrapped + * @param userModulePath The path to the file being wrapped * @returns The processed proxy module code */ -export async function rollupize(tempProxyFilePath: string, resourcePath: string): Promise { - const intermediateBundle = await rollup(getRollupInputOptions(tempProxyFilePath, resourcePath)); +export async function rollupize(tempProxyFilePath: string, userModulePath: string): Promise { + const intermediateBundle = await rollup(getRollupInputOptions(tempProxyFilePath, userModulePath)); const finalBundle = await intermediateBundle.generate(rollupOutputOptions); // The module at index 0 is always the entrypoint, which in this case is the proxy module. @@ -75,12 +75,12 @@ export async function rollupize(tempProxyFilePath: string, resourcePath: string) // square brackets into underscores. Further, Rollup adds file extensions to bare-path-type import and export sources. // Because it assumes that everything will have already been processed, it always uses `.js` as the added extension. // We need to restore the original name and extension so that Webpack will be able to find the wrapped file. - const resourceFilename = path.basename(resourcePath); - const mutatedResourceFilename = resourceFilename + const userModuleFilename = path.basename(userModulePath); + const mutatedUserModuleFilename = userModuleFilename // `[\\[\\]]` is the character class containing `[` and `]` .replace(new RegExp('[\\[\\]]', 'g'), '_') .replace(/(jsx?|tsx?)$/, 'js'); - code = code.replace(new RegExp(mutatedResourceFilename, 'g'), resourceFilename); + code = code.replace(new RegExp(mutatedUserModuleFilename, 'g'), userModuleFilename); return code; }