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

This reverts commit 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

@@ -3317,11 +3317,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"