ref(nextjs): Simplify rollupize in proxy loader (#6020)

This makes a few small changes to the `rollupize` function used by the nextjs SDK's proxy loader:

- Consolidate error handling in the proxy loader, rather than handling errors inside of `rollupize` and then handling an undefined `rollupize` return value in the proxy loader.
- Slightly simplify explanation of `makeAbsoluteExternalsRelative`
- Remove type hack in `getRollupInputOptions` which seems no longer to be needed. (It was never clear why it was necessary in the first place, and TS no longer seems mad when it's removed.)
- Rename `resourcePath` to `userModulePath` for clarity

The latter two changes are drawn from #5960, to preserve them even though it is being reverted.

Author: lobsterkatie

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<LoaderOptions>, userC
 
   // Run the proxy module code through Rollup, in order to split the `export * from '<wrapped file>'` 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

packages/nextjs/src/config/loaders/rollup.ts

@@ -1,31 +1,23 @@
-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';
 
-const getRollupInputOptions: (proxyPath: string, resourcePath: string) => RollupInputOptions = (
-  proxyPath,
-  resourcePath,
-) => ({
+const getRollupInputOptions = (proxyPath: string, userModulePath: 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
   // 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 !== 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
@@ -48,9 +40,8 @@ const getRollupInputOptions: (proxyPath: string, resourcePath: string) => Rollup
   //     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,
 });
 
@@ -65,23 +56,15 @@ const rollupOutputOptions: RollupOutputOptions = {
  * Use Rollup to process the proxy module file (located at `tempProxyFilePath`) in order to split its `export * from
  * '<wrapped file>'` 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
+ * @param userModulePath The path to the file being wrapped
+ * @returns The processed proxy module code
  */
-export async function rollupize(tempProxyFilePath: string, resourcePath: string): Promise<string | undefined> {
-  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, userModulePath: string): Promise<string> {
+  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.
   let { code } = finalBundle.output[0];
@@ -92,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;
 }