Skip to content

Commit 7e5765a

Browse files
author
Luca Forstner
committed
ref(nextjs): Use bundling instead of proxying to wrap pages and API routes
1 parent 8291249 commit 7e5765a

File tree

8 files changed

+107
-127
lines changed

8 files changed

+107
-127
lines changed

packages/nextjs/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
"access": "public"
1818
},
1919
"dependencies": {
20-
"@rollup/plugin-sucrase": "4.0.4",
20+
"@rollup/plugin-commonjs": "24.0.0",
2121
"@rollup/plugin-virtual": "3.0.0",
2222
"@sentry/core": "7.29.0",
2323
"@sentry/integrations": "7.29.0",

packages/nextjs/rollup.npm.config.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export default [
3232
// make it so Rollup calms down about the fact that we're combining default and named exports
3333
exports: 'named',
3434
},
35-
external: ['@sentry/nextjs', '__RESOURCE_PATH__'],
35+
external: ['@sentry/nextjs', '__SENTRY_WRAPEE__.cjs'],
3636
},
3737
}),
3838
),
@@ -50,7 +50,7 @@ export default [
5050
// make it so Rollup calms down about the fact that we're combining default and named exports
5151
exports: 'named',
5252
},
53-
external: ['@rollup/plugin-sucrase', 'rollup'],
53+
external: ['@rollup/plugin-commonjs', '@rollup/plugin-virtual', 'rollup'],
5454
},
5555
}),
5656
),

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

Lines changed: 8 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils';
1+
import { stringMatchesSomePattern } from '@sentry/utils';
22
import * as fs from 'fs';
33
import * as path from 'path';
44

@@ -44,48 +44,27 @@ export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userC
4444
return userCode;
4545
}
4646

47-
// We don't want to wrap twice (or infinitely), so in the proxy we add this query string onto references to the
48-
// wrapped file, so that we know that it's already been processed. (Adding this query string is also necessary to
49-
// convince webpack that it's a different file than the one it's in the middle of loading now, so that the originals
50-
// themselves will have a chance to load.)
51-
if (this.resourceQuery.includes('__sentry_wrapped__')) {
52-
return userCode;
53-
}
54-
5547
const templateFile = parameterizedRoute.startsWith('/api')
5648
? 'apiProxyLoaderTemplate.js'
5749
: 'pageProxyLoaderTemplate.js';
5850
const templatePath = path.resolve(__dirname, `../templates/${templateFile}`);
59-
let templateCode = fs.readFileSync(templatePath).toString();
51+
let templateCode = fs.readFileSync(templatePath, { encoding: 'utf8' });
52+
6053
// Make sure the template is included when running `webpack watch`
6154
this.addDependency(templatePath);
6255

6356
// Inject the route and the path to the file we're wrapping into the template
6457
templateCode = templateCode.replace(/__ROUTE__/g, parameterizedRoute.replace(/\\/g, '\\\\'));
65-
templateCode = templateCode.replace(/__RESOURCE_PATH__/g, this.resourcePath.replace(/\\/g, '\\\\'));
6658

6759
// Run the proxy module code through Rollup, in order to split the `export * from '<wrapped file>'` out into
6860
// individual exports (which nextjs seems to require).
69-
let proxyCode;
7061
try {
71-
proxyCode = await rollupize(templateCode, this.resourcePath);
62+
return await rollupize(templateCode, userCode);
7263
} catch (err) {
73-
__DEBUG_BUILD__ &&
74-
logger.warn(
75-
`Could not wrap ${this.resourcePath}. An error occurred while processing the proxy module template:\n${err}`,
76-
);
64+
// eslint-disable-next-line no-console
65+
console.warn(
66+
`[@sentry/nextjs] Could not instrument ${this.resourcePath}. An error occurred while auto-wrapping:\n${err}`,
67+
);
7768
return userCode;
7869
}
79-
80-
// Add a query string onto all references to the wrapped file, so that webpack will consider it different from the
81-
// non-query-stringged version (which we're already in the middle of loading as we speak), and load it separately from
82-
// this. When the second load happens this loader will run again, but we'll be able to see the query string and will
83-
// know to immediately return without processing. This avoids an infinite loop.
84-
const resourceFilename = path.basename(this.resourcePath);
85-
proxyCode = proxyCode.replace(
86-
new RegExp(`/${escapeStringForRegex(resourceFilename)}'`, 'g'),
87-
`/${resourceFilename}?__sentry_wrapped__'`,
88-
);
89-
90-
return proxyCode;
9170
}
Lines changed: 61 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -1,104 +1,78 @@
1-
import sucrase from '@rollup/plugin-sucrase';
1+
import commonjs from '@rollup/plugin-commonjs';
22
import virtual from '@rollup/plugin-virtual';
3-
import { escapeStringForRegex } from '@sentry/utils';
4-
import * as path from 'path';
5-
import type { InputOptions as RollupInputOptions, OutputOptions as RollupOutputOptions } from 'rollup';
63
import { rollup } from 'rollup';
74

8-
const SENTRY_PROXY_MODULE_NAME = 'sentry-proxy-module';
9-
10-
const getRollupInputOptions = (templateCode: string, userModulePath: string): RollupInputOptions => ({
11-
input: SENTRY_PROXY_MODULE_NAME,
12-
13-
plugins: [
14-
virtual({
15-
[SENTRY_PROXY_MODULE_NAME]: templateCode,
16-
}),
17-
18-
sucrase({
19-
transforms: ['jsx', 'typescript'],
20-
}),
21-
],
22-
23-
// 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
24-
// proxy module (living in the temporary file we've created) and the file we're wrapping not to be external, because
25-
// otherwise they won't be processed. (We need Rollup to process the former so that we can use the code, and we need
26-
// it to process the latter so it knows what exports to re-export from the proxy module.) Past that, we don't care, so
27-
// don't bother to process anything else.
28-
external: importPath => importPath !== SENTRY_PROXY_MODULE_NAME && importPath !== userModulePath,
29-
30-
// Prevent rollup from stressing out about TS's use of global `this` when polyfilling await. (TS will polyfill if the
31-
// user's tsconfig `target` is set to anything before `es2017`. See https://stackoverflow.com/a/72822340 and
32-
// https://stackoverflow.com/a/60347490.)
33-
context: 'this',
34-
35-
// Rollup's path-resolution logic when handling re-exports can go wrong when wrapping pages which aren't at the root
36-
// level of the `pages` directory. This may be a bug, as it doesn't match the behavior described in the docs, but what
37-
// seems to happen is this:
38-
//
39-
// - We try to wrap `pages/xyz/userPage.js`, which contains `export { helperFunc } from '../../utils/helper'`
40-
// - Rollup converts '../../utils/helper' into an absolute path
41-
// - We mark the helper module as external
42-
// - Rollup then converts it back to a relative path, but relative to `pages/` rather than `pages/xyz/`. (This is
43-
// the part which doesn't match the docs. They say that Rollup will use the common ancestor of all modules in the
44-
// bundle as the basis for the relative path calculation, but both our temporary file and the page being wrapped
45-
// live in `pages/xyz/`, and they're the only two files in the bundle, so `pages/xyz/`` should be used as the
46-
// root. Unclear why it's not.)
47-
// - As a result of the miscalculation, our proxy module will include `export { helperFunc } from '../utils/helper'`
48-
// rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in
49-
// nextjs..
50-
//
51-
// Setting `makeAbsoluteExternalsRelative` to `false` prevents all of the above by causing Rollup to ignore imports of
52-
// externals entirely, with the result that their paths remain untouched (which is what we want).
53-
makeAbsoluteExternalsRelative: false,
54-
});
55-
56-
const rollupOutputOptions: RollupOutputOptions = {
57-
format: 'esm',
58-
59-
// Don't create a bundle - we just want the transformed entrypoint file
60-
preserveModules: true,
61-
};
5+
const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module';
6+
const WRAPEE_MODULE_NAME = '__SENTRY_WRAPEE__.cjs';
627

638
/**
649
* Use Rollup to process the proxy module code, in order to split its `export * from '<wrapped file>'` call into
6510
* individual exports (which nextjs seems to need).
6611
*
67-
* Note: Any errors which occur are handled by the proxy loader which calls this function.
12+
* Note: This function may throw in case something goes wrong while bundling.
6813
*
69-
* @param templateCode The proxy module code
70-
* @param userModulePath The path to the file being wrapped
14+
* @param templateCode The wrapper module code
15+
* @param userModuleCode The path to the file being wrapped
7116
* @returns The processed proxy module code
7217
*/
73-
export async function rollupize(templateCode: string, userModulePath: string): Promise<string> {
74-
const intermediateBundle = await rollup(getRollupInputOptions(templateCode, userModulePath));
75-
const finalBundle = await intermediateBundle.generate(rollupOutputOptions);
18+
export async function rollupize(templateCode: string, userModuleCode: string): Promise<string> {
19+
const rollupBuild = await rollup({
20+
input: SENTRY_WRAPPER_MODULE_NAME,
7621

77-
// The module at index 0 is always the entrypoint, which in this case is the proxy module.
78-
let { code } = finalBundle.output[0];
22+
plugins: [
23+
// We're using virtual modules so we don't have to mess around with file paths
24+
virtual({
25+
[SENTRY_WRAPPER_MODULE_NAME]: templateCode,
26+
[WRAPEE_MODULE_NAME]: userModuleCode,
27+
}),
28+
29+
// People may use `module.exports` in their API routes or page files. Next.js allows that and we also need to
30+
// handle that correctly so we let a plugin to take care of bundling cjs exports for us.
31+
commonjs(),
32+
],
33+
34+
// We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external.
35+
external: source => source !== SENTRY_WRAPPER_MODULE_NAME && source !== WRAPEE_MODULE_NAME,
7936

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>'`.
82-
//
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.)
37+
// Prevent rollup from stressing out about TS's use of global `this` when polyfilling await. (TS will polyfill if the
38+
// user's tsconfig `target` is set to anything before `es2017`. See https://stackoverflow.com/a/72822340 and
39+
// https://stackoverflow.com/a/60347490.)
40+
context: 'this',
9241

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];
42+
// Rollup's path-resolution logic when handling re-exports can go wrong when wrapping pages which aren't at the root
43+
// level of the `pages` directory. This may be a bug, as it doesn't match the behavior described in the docs, but what
44+
// seems to happen is this:
45+
//
46+
// - We try to wrap `pages/xyz/userPage.js`, which contains `export { helperFunc } from '../../utils/helper'`
47+
// - Rollup converts '../../utils/helper' into an absolute path
48+
// - We mark the helper module as external
49+
// - Rollup then converts it back to a relative path, but relative to `pages/` rather than `pages/xyz/`. (This is
50+
// the part which doesn't match the docs. They say that Rollup will use the common ancestor of all modules in the
51+
// bundle as the basis for the relative path calculation, but both our temporary file and the page being wrapped
52+
// live in `pages/xyz/`, and they're the only two files in the bundle, so `pages/xyz/`` should be used as the
53+
// root. Unclear why it's not.)
54+
// - As a result of the miscalculation, our proxy module will include `export { helperFunc } from '../utils/helper'`
55+
// rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in
56+
// nextjs..
57+
//
58+
// Setting `makeAbsoluteExternalsRelative` to `false` prevents all of the above by causing Rollup to ignore imports of
59+
// externals entirely, with the result that their paths remain untouched (which is what we want).
60+
makeAbsoluteExternalsRelative: false,
10061

101-
code = code.replace(new RegExp(escapeStringForRegex(messedUpPath), 'g'), `./${path.basename(userModulePath)}`);
62+
onwarn: () => {
63+
// Suppress all warnings - we don't want to bother people with this output
64+
},
10265

103-
return code;
66+
output: {
67+
interop: 'auto',
68+
exports: 'named',
69+
},
70+
});
71+
72+
const finalBundle = await rollupBuild.generate({
73+
format: 'esm',
74+
});
75+
76+
// The module at index 0 is always the entrypoint, which in this case is the proxy module.
77+
return finalBundle.output[0].code;
10478
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
// @ts-ignore See above
1010
// eslint-disable-next-line import/no-unresolved
11-
import * as origModule from '__RESOURCE_PATH__';
11+
import * as origModule from '__SENTRY_WRAPEE__.cjs';
1212
import * as Sentry from '@sentry/nextjs';
1313
import type { PageConfig } from 'next';
1414

@@ -73,4 +73,4 @@ export default exportedHandler;
7373
// not include anything whose name matchs something we've explicitly exported above.
7474
// @ts-ignore See above
7575
// eslint-disable-next-line import/no-unresolved
76-
export * from '__RESOURCE_PATH__';
76+
export * from '__SENTRY_WRAPEE__.cjs';

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
// @ts-ignore See above
1010
// eslint-disable-next-line import/no-unresolved
11-
import * as wrapee from '__RESOURCE_PATH__';
11+
import * as wrapee from '__SENTRY_WRAPEE__.cjs';
1212
import * as Sentry from '@sentry/nextjs';
1313
import type { GetServerSideProps, GetStaticProps, NextPage as NextPageComponent } from 'next';
1414

@@ -53,4 +53,4 @@ export default pageComponent;
5353
// not include anything whose name matchs something we've explicitly exported above.
5454
// @ts-ignore See above
5555
// eslint-disable-next-line import/no-unresolved
56-
export * from '__RESOURCE_PATH__';
56+
export * from '__SENTRY_WRAPEE__.cjs';

packages/nextjs/src/config/webpack.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ export function constructWebpackConfigFunction(
9393
const pageExtensions = userNextConfig.pageExtensions || ['tsx', 'ts', 'jsx', 'js'];
9494
const pageExtensionRegex = pageExtensions.map(escapeStringForRegex).join('|');
9595

96-
newConfig.module.rules.push({
96+
// It is very important that we insert our loader at the beginning of the array because we expect any sort of transformations/transpilations (e.g. TS -> JS) to already have happened.
97+
newConfig.module.rules.unshift({
9798
test: new RegExp(`^${escapeStringForRegex(pagesDir)}.*\\.(${pageExtensionRegex})$`),
9899
use: [
99100
{

yarn.lock

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3385,6 +3385,18 @@
33853385
dependencies:
33863386
web-streams-polyfill "^3.1.1"
33873387

3388+
3389+
version "24.0.0"
3390+
resolved "https://registry.yarnpkg.com/@rollup/plugin-commonjs/-/plugin-commonjs-24.0.0.tgz#fb7cf4a6029f07ec42b25daa535c75b05a43f75c"
3391+
integrity sha512-0w0wyykzdyRRPHOb0cQt14mIBLujfAv6GgP6g8nvg/iBxEm112t3YPPq+Buqe2+imvElTka+bjNlJ/gB56TD8g==
3392+
dependencies:
3393+
"@rollup/pluginutils" "^5.0.1"
3394+
commondir "^1.0.1"
3395+
estree-walker "^2.0.2"
3396+
glob "^8.0.3"
3397+
is-reference "1.2.1"
3398+
magic-string "^0.27.0"
3399+
33883400
"@rollup/plugin-commonjs@^15.0.0":
33893401
version "15.1.0"
33903402
resolved "https://registry.yarnpkg.com/@rollup/plugin-commonjs/-/plugin-commonjs-15.1.0.tgz#1e7d076c4f1b2abf7e65248570e555defc37c238"
@@ -3450,7 +3462,7 @@
34503462
"@rollup/pluginutils" "^3.1.0"
34513463
magic-string "^0.25.7"
34523464

3453-
"@rollup/plugin-sucrase@4.0.4", "@rollup/plugin-sucrase@^4.0.3":
3465+
"@rollup/plugin-sucrase@^4.0.3":
34543466
version "4.0.4"
34553467
resolved "https://registry.yarnpkg.com/@rollup/plugin-sucrase/-/plugin-sucrase-4.0.4.tgz#0a3b3d97cdc239ec3399f5a10711f751e9f95d98"
34563468
integrity sha512-YH4J8yoJb5EVnLhAwWxYAQNh2SJOR+SdZ6XdgoKEv6Kxm33riYkM8MlMaggN87UoISP52qAFyZ5ey56wu6umGg==
@@ -3488,6 +3500,15 @@
34883500
estree-walker "^2.0.1"
34893501
picomatch "^2.2.2"
34903502

3503+
"@rollup/pluginutils@^5.0.1":
3504+
version "5.0.2"
3505+
resolved "https://registry.yarnpkg.com/@rollup/pluginutils/-/pluginutils-5.0.2.tgz#012b8f53c71e4f6f9cb317e311df1404f56e7a33"
3506+
integrity sha512-pTd9rIsP92h+B6wWwFbW8RkZv4hiR/xKsqre4SIuAOaOEQRxi0lqLke9k2/7WegC85GgUs9pjmOjCUi3In4vwA==
3507+
dependencies:
3508+
"@types/estree" "^1.0.0"
3509+
estree-walker "^2.0.2"
3510+
picomatch "^2.3.1"
3511+
34913512
"@schematics/[email protected]":
34923513
version "10.2.4"
34933514
resolved "https://registry.yarnpkg.com/@schematics/angular/-/angular-10.2.4.tgz#3b99b9da572b57381d221e2008804e6bb9c98b82"
@@ -4081,6 +4102,11 @@
40814102
resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.39.tgz#e177e699ee1b8c22d23174caaa7422644389509f"
40824103
integrity sha512-EYNwp3bU+98cpU4lAWYYL7Zz+2gryWH1qbdDTidVd6hkiR6weksdbMadyXKXNPEkQFhXM+hVO9ZygomHXp+AIw==
40834104

4105+
"@types/estree@^1.0.0":
4106+
version "1.0.0"
4107+
resolved "https://registry.yarnpkg.com/@types/estree/-/estree-1.0.0.tgz#5fb2e536c1ae9bf35366eed879e827fa59ca41c2"
4108+
integrity sha512-WulqXMDUTYAXCjZnk6JtIHPigp55cVtDgDrO2gHRwhyJto21+1zbVCtOYB2L1F9w4qCQ0rOGWBnBe0FNTiEJIQ==
4109+
40844110
"@types/[email protected]", "@types/[email protected]", "@types/express-serve-static-core@^4.17.18":
40854111
version "4.17.30"
40864112
resolved "https://registry.yarnpkg.com/@types/express-serve-static-core/-/express-serve-static-core-4.17.30.tgz#0f2f99617fa8f9696170c46152ccf7500b34ac04"
@@ -12742,7 +12768,7 @@ [email protected]:
1274212768
once "^1.3.0"
1274312769
path-is-absolute "^1.0.0"
1274412770

12745-
12771+
[email protected], glob@^8.0.3:
1274612772
version "8.0.3"
1274712773
resolved "https://registry.yarnpkg.com/glob/-/glob-8.0.3.tgz#415c6eb2deed9e502c68fa44a272e6da6eeca42e"
1274812774
integrity sha512-ull455NHSHI/Y1FqGaaYFaLGkNMMJbavMrEGFXG/PGrg6y7sutWHUHrz6gy6WEBH6akM1M414dWKCNs+IhKdiQ==
@@ -14359,7 +14385,7 @@ is-potential-custom-element-name@^1.0.1:
1435914385
resolved "https://registry.yarnpkg.com/is-potential-custom-element-name/-/is-potential-custom-element-name-1.0.1.tgz#171ed6f19e3ac554394edf78caa05784a45bebb5"
1436014386
integrity sha512-bCYeRA2rVibKZd+s2625gGnGF/t7DSqDs4dP7CrLA1m7jKWz6pps0LpYLJN8Q64HtmPKJ1hrN3nzPNKFEKOUiQ==
1436114387

14362-
is-reference@^1.2.1:
14388+
is-reference@1.2.1, is-reference@^1.2.1:
1436314389
version "1.2.1"
1436414390
resolved "https://registry.yarnpkg.com/is-reference/-/is-reference-1.2.1.tgz#8b2dac0b371f4bc994fdeaba9eb542d03002d0b7"
1436514391
integrity sha512-U82MsXXiFIrjCK4otLT+o2NA2Cd2g5MLoOVXUZjIOhLurrRxpEXzI8O0KZHr3IjLvlAH1kTPYSuqer5T9ZVBKQ==

0 commit comments

Comments
 (0)