Skip to content

Commit 3ff9527

Browse files
authored
ref(nextjs): Use virtual rather than temporary file for proxy module (#6021)
In the proxy loader used by the nextjs SDK to autowrap user code, we currently write a temporary file to disk for each proxy module we create, because by default Rollup's API expects a path to a file, not the file's contents. It's a little bit of an ugly hack, though, and can cause problems because it writes to the user's source directory rather than to their build directory. This switches to the use of virtual files, using the `@rollup/virtual-plugin` plugin. For unclear reasons, this seems to change (and not in a good way) the base of the relative paths calculated by Rollup when transforming the proxy templates' `import * as wrappedFile from <wrapped file>` and `export * from <wrapped file>` statements. We already have to manually fix those statements, to undo the fact that Rollup substitutes underscores for any square brackets which appear in the paths, but that fix is straightforward because it's very easy to predict what the wrong version will look like, so finding it and overwriting it with the right version is easy. With the relative path base change introduced by the virtual plugin it's not as simple, though, because there's not an easily-discernible pattern to what new base gets picked. (Sometimes it's `pages/`, sometimes it's `pages/api`, sometimes the path uses `./xxx`, sometimes the path uses `../xxx`...) Therefore, rather than trying to nail down and then handle each case of that logic in order to _predict_ what the incorrect path will be, this looks for the transformed version of the whole `import * as wrappedFile from <wrapped file>` statement and reads the incorrect path from there using a regex. Note: This is a second attempt at #5960, which was reverted. H/t to the author of that PR, @lforst, for rubber-ducking this new solution. Fixes #5944.
1 parent 51c1191 commit 3ff9527

File tree

4 files changed

+46
-35
lines changed

4 files changed

+46
-35
lines changed

Diff for: packages/nextjs/package.json

+1
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
},
1919
"dependencies": {
2020
"@rollup/plugin-sucrase": "4.0.4",
21+
"@rollup/plugin-virtual": "3.0.0",
2122
"@sentry/core": "7.16.0",
2223
"@sentry/integrations": "7.16.0",
2324
"@sentry/node": "7.16.0",

Diff for: packages/nextjs/src/config/loaders/proxyLoader.ts

+3-16
Original file line numberDiff line numberDiff line change
@@ -50,34 +50,21 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
5050
// Make sure the template is included when runing `webpack watch`
5151
this.addDependency(templatePath);
5252

53-
// Inject the route into the template
53+
// Inject the route and the path to the file we're wrapping into the template
5454
templateCode = templateCode.replace(/__ROUTE__/g, parameterizedRoute);
55-
56-
// Fill in the path to the file we're wrapping and save the result as a temporary file in the same folder (so that
57-
// relative imports and exports are calculated correctly).
58-
//
59-
// TODO: We're saving the filled-in template to disk, however temporarily, because Rollup expects a path to a code
60-
// file, not code itself. There is a rollup plugin which can fake this (`@rollup/plugin-virtual`) but the virtual file
61-
// seems to be inside of a virtual directory (in other words, one level down from where you'd expect it) and that
62-
// messes up relative imports and exports. Presumably there's a way to make it work, though, and if we can, it would
63-
// be cleaner than having to first write and then delete a temporary file each time we run this loader.
6455
templateCode = templateCode.replace(/__RESOURCE_PATH__/g, this.resourcePath);
65-
const tempFilePath = path.resolve(path.dirname(this.resourcePath), `temp${Math.random()}.js`);
66-
fs.writeFileSync(tempFilePath, templateCode);
6756

6857
// Run the proxy module code through Rollup, in order to split the `export * from '<wrapped file>'` out into
69-
// individual exports (which nextjs seems to require), then delete the tempoary file.
58+
// individual exports (which nextjs seems to require).
7059
let proxyCode;
7160
try {
72-
proxyCode = await rollupize(tempFilePath, this.resourcePath);
61+
proxyCode = await rollupize(templateCode, this.resourcePath);
7362
} catch (err) {
7463
__DEBUG_BUILD__ &&
7564
logger.warn(
7665
`Could not wrap ${this.resourcePath}. An error occurred while processing the proxy module template:\n${err}`,
7766
);
7867
return userCode;
79-
} finally {
80-
fs.unlinkSync(tempFilePath);
8168
}
8269

8370
// Add a query string onto all references to the wrapped file, so that webpack will consider it different from the

Diff for: packages/nextjs/src/config/loaders/rollup.ts

+37-19
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,20 @@
11
import sucrase from '@rollup/plugin-sucrase';
2+
import virtual from '@rollup/plugin-virtual';
3+
import { escapeStringForRegex } from '@sentry/utils';
24
import * as path from 'path';
35
import type { InputOptions as RollupInputOptions, OutputOptions as RollupOutputOptions } from 'rollup';
46
import { rollup } from 'rollup';
57

6-
const getRollupInputOptions = (proxyPath: string, userModulePath: string): RollupInputOptions => ({
7-
input: proxyPath,
8+
const SENTRY_PROXY_MODULE_NAME = 'sentry-proxy-module';
9+
10+
const getRollupInputOptions = (templateCode: string, userModulePath: string): RollupInputOptions => ({
11+
input: SENTRY_PROXY_MODULE_NAME,
812

913
plugins: [
14+
virtual({
15+
[SENTRY_PROXY_MODULE_NAME]: templateCode,
16+
}),
17+
1018
sucrase({
1119
transforms: ['jsx', 'typescript'],
1220
}),
@@ -17,7 +25,7 @@ const getRollupInputOptions = (proxyPath: string, userModulePath: string): Rollu
1725
// otherwise they won't be processed. (We need Rollup to process the former so that we can use the code, and we need
1826
// it to process the latter so it knows what exports to re-export from the proxy module.) Past that, we don't care, so
1927
// don't bother to process anything else.
20-
external: importPath => importPath !== proxyPath && importPath !== userModulePath,
28+
external: importPath => importPath !== SENTRY_PROXY_MODULE_NAME && importPath !== userModulePath,
2129

2230
// Prevent rollup from stressing out about TS's use of global `this` when polyfilling await. (TS will polyfill if the
2331
// user's tsconfig `target` is set to anything before `es2017`. See https://stackoverflow.com/a/72822340 and
@@ -53,34 +61,44 @@ const rollupOutputOptions: RollupOutputOptions = {
5361
};
5462

5563
/**
56-
* Use Rollup to process the proxy module file (located at `tempProxyFilePath`) in order to split its `export * from
57-
* '<wrapped file>'` call into individual exports (which nextjs seems to need).
64+
* Use Rollup to process the proxy module code, in order to split its `export * from '<wrapped file>'` call into
65+
* individual exports (which nextjs seems to need).
5866
*
5967
* Note: Any errors which occur are handled by the proxy loader which calls this function.
6068
*
61-
* @param tempProxyFilePath The path to the temporary file containing the proxy module code
69+
* @param templateCode The proxy module code
6270
* @param userModulePath The path to the file being wrapped
6371
* @returns The processed proxy module code
6472
*/
65-
export async function rollupize(tempProxyFilePath: string, userModulePath: string): Promise<string> {
66-
const intermediateBundle = await rollup(getRollupInputOptions(tempProxyFilePath, userModulePath));
73+
export async function rollupize(templateCode: string, userModulePath: string): Promise<string> {
74+
const intermediateBundle = await rollup(getRollupInputOptions(templateCode, userModulePath));
6775
const finalBundle = await intermediateBundle.generate(rollupOutputOptions);
6876

6977
// The module at index 0 is always the entrypoint, which in this case is the proxy module.
7078
let { code } = finalBundle.output[0];
7179

72-
// Rollup does a few things to the code we *don't* want. Undo those changes before returning the code.
80+
// In addition to doing the desired work, Rollup also does a few things we *don't* want. Specifically, in messes up
81+
// the path in both `import * as origModule from '<userModulePath>'` and `export * from '<userModulePath>'`.
7382
//
74-
// Nextjs uses square brackets surrounding a path segment to denote a parameter in the route, but Rollup turns those
75-
// square brackets into underscores. Further, Rollup adds file extensions to bare-path-type import and export sources.
76-
// Because it assumes that everything will have already been processed, it always uses `.js` as the added extension.
77-
// We need to restore the original name and extension so that Webpack will be able to find the wrapped file.
78-
const userModuleFilename = path.basename(userModulePath);
79-
const mutatedUserModuleFilename = userModuleFilename
80-
// `[\\[\\]]` is the character class containing `[` and `]`
81-
.replace(new RegExp('[\\[\\]]', 'g'), '_')
82-
.replace(/(jsx?|tsx?)$/, 'js');
83-
code = code.replace(new RegExp(mutatedUserModuleFilename, 'g'), userModuleFilename);
83+
// - It turns the square brackets surrounding each parameterized path segment into underscores.
84+
// - It always adds `.js` to the end of the filename.
85+
// - It converts the path from aboslute to relative, which would be fine except that when used with the virual plugin,
86+
// it uses an incorrect (and not entirely predicable) base for that relative path.
87+
//
88+
// To fix this, we overwrite the messed up path with what we know it should be: `./<userModulePathBasename>`. (We can
89+
// find the value of the messed up path by looking at what `import * as origModule from '<userModulePath>'` becomes.
90+
// Because it's the first line of the template, it's also the first line of the result, and is therefore easy to
91+
// find.)
92+
93+
const importStarStatement = code.split('\n')[0];
94+
// This regex should always match (we control both the input and the process which generates it, so we can guarantee
95+
// the outcome of that processing), but just in case it somehow doesn't, we need it to throw an error so that the
96+
// proxy loader will know to return the user's code untouched rather than returning proxy module code including a
97+
// broken path. The non-null assertion asserts that a match has indeed been found.
98+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
99+
const messedUpPath = /^import \* as .* from '(.*)';$/.exec(importStarStatement)![1];
100+
101+
code = code.replace(new RegExp(escapeStringForRegex(messedUpPath), 'g'), `./${path.basename(userModulePath)}`);
84102

85103
return code;
86104
}

Diff for: yarn.lock

+5
Original file line numberDiff line numberDiff line change
@@ -4231,6 +4231,11 @@
42314231
"@rollup/pluginutils" "^4.1.1"
42324232
sucrase "^3.20.0"
42334233

4234+
"@rollup/plugin-virtual@3.0.0":
4235+
version "3.0.0"
4236+
resolved "https://registry.yarnpkg.com/@rollup/plugin-virtual/-/plugin-virtual-3.0.0.tgz#8c3f54b4ab4b267d9cd3dcbaedc58d4fd1deddca"
4237+
integrity sha512-K9KORe1myM62o0lKkNR4MmCxjwuAXsZEtIHpaILfv4kILXTOrXt/R2ha7PzMcCHPYdnkWPiBZK8ed4Zr3Ll5lQ==
4238+
42344239
"@rollup/pluginutils@^3.0.8", "@rollup/pluginutils@^3.0.9", "@rollup/pluginutils@^3.1.0":
42354240
version "3.1.0"
42364241
resolved "https://registry.yarnpkg.com/@rollup/pluginutils/-/pluginutils-3.1.0.tgz#706b4524ee6dc8b103b3c995533e5ad680c02b9b"

0 commit comments

Comments
 (0)