Revert "ref(nextjs): Use virtual file for proxying in proxy loader (#5960)" (#6018)

In #5960, we switched from writing temporary files to disk to using virtual files when creating proxy modules in the nextjs SDK's proxy loader. Though it was a good change, it didn't handle certain cases with the potential to crash a user's build process.

Specifically, rollup converts the absolute paths in the template's `import * as wrappedFile from <wrapped file>` and `export * from <wrapped file>` into relative paths, but can break them in the process, by doing things like replacing `[` and `]` with `_`.  Prior to the switch to virtual files, those relative paths only ever had one segment, and so the code compensating for rollup's weirdness only handles basenames. After the switch, the relative paths sometimes have multiple segments, in a pattern which isn't easy to predict. When that happens, the aforementioned compensation code fails and the broken relative paths aren't fixed.

After discussing with @lforst, the author of that PR, we agreed to take a different approach to handling the broken paths. Rather than add a fix on top of that change, I'm choosing to revert it and implement the new approach separately, so that the new work is easier to review and the change is easier to reason about. Otherwise, the fix would include a bunch of undoing of changes, which would muddy the waters. #5960 also contained a few small refactors which don't bear directly on the switch to using virtual files, and reverting allows me to pull those out into a separate PR, again for ease of understanding.

Reverts 2d068e9.

Author: lobsterkatie

packages/nextjs/package.json

@@ -18,7 +18,6 @@
   },
   "dependencies": {
     "@rollup/plugin-sucrase": "4.0.4",
-    "@rollup/plugin-virtual": "3.0.0",
     "@sentry/core": "7.16.0",
     "@sentry/integrations": "7.16.0",
     "@sentry/node": "7.16.0",

packages/nextjs/rollup.npm.config.js

@@ -33,7 +33,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', '__RESOURCE_PATH__'],
       },
     }),
   ),

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

@@ -38,7 +38,7 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
   // 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__') || this.resourceQuery.includes('__sentry_external__')) {
+  if (this.resourceQuery.includes('__sentry_wrapped__')) {
     return userCode;
   }
 
@@ -55,29 +55,34 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
 
   // Fill in the path to the file we're wrapping and save the result as a temporary file in the same folder (so that
   // relative imports and exports are calculated correctly).
+  //
+  // TODO: We're saving the filled-in template to disk, however temporarily, because Rollup expects a path to a code
+  // file, not code itself. There is a rollup plugin which can fake this (`@rollup/plugin-virtual`) but the virtual file
+  // seems to be inside of a virtual directory (in other words, one level down from where you'd expect it) and that
+  // messes up relative imports and exports. Presumably there's a way to make it work, though, and if we can, it would
+  // be cleaner than having to first write and then delete a temporary file each time we run this loader.
   templateCode = templateCode.replace(/__RESOURCE_PATH__/g, this.resourcePath);
+  const tempFilePath = path.resolve(path.dirname(this.resourcePath), `temp${Math.random()}.js`);
+  fs.writeFileSync(tempFilePath, templateCode);
 
   // 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(this.resourcePath, templateCode);
+  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
     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);
-
-  // For some reason when using virtual files (via the @rollup/plugin-virtual), rollup will always resolve imports with
-  // absolute imports to relative imports with `..`.In our case we need`.`, which is why we're replacing for that here.
-  // Also, we're adding 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.
   proxyCode = proxyCode.replace(
-    new RegExp(`'../${escapeStringForRegex(resourceFilename)}'`, 'g'),
-    `'./${resourceFilename}?__sentry_wrapped__'`,
+    new RegExp(`/${escapeStringForRegex(resourceFilename)}'`, 'g'),
+    `/${resourceFilename}?__sentry_wrapped__'`,
   );
 
   return proxyCode;

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

@@ -1,30 +1,31 @@
+import type { RollupSucraseOptions } from '@rollup/plugin-sucrase';
 import sucrase from '@rollup/plugin-sucrase';
-import virtual from '@rollup/plugin-virtual';
 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 SENTRY_PROXY_MODULE_NAME = 'sentry-proxy-module';
-
-const getRollupInputOptions = (userModulePath: string, proxyTemplateCode: string): RollupInputOptions => ({
-  input: SENTRY_PROXY_MODULE_NAME,
-
+const getRollupInputOptions: (proxyPath: string, resourcePath: string) => RollupInputOptions = (
+  proxyPath,
+  resourcePath,
+) => ({
+  input: proxyPath,
   plugins: [
-    virtual({
-      [SENTRY_PROXY_MODULE_NAME]: proxyTemplateCode,
-    }),
+    // 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 !== SENTRY_PROXY_MODULE_NAME && importPath !== userModulePath,
+  external: importPath => importPath !== proxyPath && importPath !== resourcePath,
 
   // 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
@@ -65,19 +66,19 @@ const rollupOutputOptions: RollupOutputOptions = {
  * '<wrapped file>'` call into individual exports (which nextjs seems to need).
  *
  * @param tempProxyFilePath The path to the temporary file containing the proxy module code
- * @param userModulePath The path to the file being wrapped
+ * @param resourcePath The path to the file being wrapped
  * @returns The processed proxy module code, or undefined if an error occurs
  */
-export async function rollupize(userModulePath: string, templateCode: string): Promise<string | undefined> {
+export async function rollupize(tempProxyFilePath: string, resourcePath: string): Promise<string | undefined> {
   let finalBundle;
 
   try {
-    const intermediateBundle = await rollup(getRollupInputOptions(userModulePath, templateCode));
+    const intermediateBundle = await rollup(getRollupInputOptions(tempProxyFilePath, resourcePath));
     finalBundle = await intermediateBundle.generate(rollupOutputOptions);
   } catch (err) {
     __DEBUG_BUILD__ &&
       logger.warn(
-        `Could not wrap ${userModulePath}. An error occurred while processing the proxy module template:\n${err}`,
+        `Could not wrap ${resourcePath}. An error occurred while processing the proxy module template:\n${err}`,
       );
     return undefined;
   }
@@ -91,7 +92,7 @@ export async function rollupize(userModulePath: string, templateCode: string): P
   // 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(userModulePath);
+  const resourceFilename = path.basename(resourcePath);
   const mutatedResourceFilename = resourceFilename
     // `[\\[\\]]` is the character class containing `[` and `]`
     .replace(new RegExp('[\\[\\]]', 'g'), '_')

packages/nextjs/src/config/templates/apiProxyLoaderTemplate.ts

@@ -4,15 +4,11 @@
  *
  * We use `__RESOURCE_PATH__` 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.
- *
- * The `?__sentry_external__` is used to
- * 1) tell rollup to treat the import as external (i.e. not process it)
- * 2) tell webpack not to proxy this file again (avoiding an infinite loop)
  */
 
 // @ts-ignore See above
 // eslint-disable-next-line import/no-unresolved
-import * as origModule from '__RESOURCE_PATH__?__sentry_external__';
+import * as origModule from '__RESOURCE_PATH__';
 import * as Sentry from '@sentry/nextjs';
 import type { PageConfig } from 'next';
 

packages/nextjs/src/config/templates/pageProxyLoaderTemplate.ts

@@ -4,15 +4,11 @@
  *
  * We use `__RESOURCE_PATH__` 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.
- *
- * The `?__sentry_external__` is used to
- * 1) tell rollup to treat the import as external (i.e. not process it)
- * 2) tell webpack not to proxy this file again (avoiding an infinite loop)
  */
 
 // @ts-ignore See above
 // eslint-disable-next-line import/no-unresolved
-import * as wrapee from '__RESOURCE_PATH__?__sentry_external__';
+import * as wrapee from '__RESOURCE_PATH__';
 import * as Sentry from '@sentry/nextjs';
 import type { GetServerSideProps, GetStaticProps, NextPage as NextPageComponent } from 'next';
 

yarn.lock

@@ -3351,11 +3351,6 @@
     "@rollup/pluginutils" "^4.1.1"
     sucrase "^3.20.0"
 
-"@rollup/[email protected]":
-  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"