Skip to content

Commit f7241c4

Browse files
authored
ref(nextjs): Use proxy loader for wrapping all data-fetching functions (#5602)
This changes the experimental auto-wrapping feature to use a templated proxy module for all wrapping. (Previously, the wrapping was done using a different mix of code parsing, AST manipulation, proxying, templating, and string concatenation for each function.) Not only should this be easier to reason about and maintain (because it's one unified method of wrapping), it also solves a number of the disadvantages inherent in various parts of the previous approach. Specifically, using a template for everything rather than storing code as strings lets us take advantage of linting and syntax highlighting, and using a proxy loader rather than dealing with the AST directly puts the onus of handling syntax variations and edge cases on tools which are actually designed for that purpose. At a high level, the proxy loader works like this: - During the nextjs build phase, webpack loads all of the files in the `pages` directory, and feeds each one to our loader. - The loader derives the parameterized route from the page file's path, and fills it and the path itself into the template. - In the template, we load the page module, replace any data-fetching methods we find with wrapped versions of themselves, and then re-export everything. - The contents of the template is returned by the loader in place of the original contents of the page module. Previously, when working directly with the page module's AST, we had to account for the very many ways functions can be defined and exported. By contrast, doing the function wrapping in a separate module allows us to take advantage of the fact that imported modules have a single, known structure, which we can modify directly in the template code. Notes: - For some reason, nextjs won't accept data fetchers which are exported as part of an `export * from '...'` statement. Therefore, the "re-export everything" part of the third step above needs to be of the form `export { all, of, the, things, which, the, page, module, exports, listed, individually } from './pageModule'`. This in turn requires knowing the full list of each page module's exports, since, unfortunately, `export { ...importedPageModule }` isn't a thing. As it turns out, one of the noticeable differences between our published code before and after the build process revamp in the spring is that where `tsc` leaves `export *` statements untouched, Rollup splits them out into individual exports - exactly what's needed here! The loader therefore uses Rollup's JS API to process the proxy module code before returning it. Further, in order that Rollup be able to understand the page module code (which will be written in either `jsx` or `tsx`), we first use Sucrase to transpile the code to vanilla JS. Who knew the build process work would come in so handy? - Given that we replace a page module's contents with the proxy code the first time webpack tries to load it, we need webpack to load the same module a second time, in order to be able to process and bundle the page module itself. We therefore attach a query string to the end of the page module's path wherever it's referenced in the template, because this makes Webpack think it is a different, as-yet-unloaded module, causing it to perform the second load. The query string also acts like a flag for us, so that the second time through we know we've already handled the file and can let it pass through the loader untouched. - Rollup expects the entry point to be given as a path to a file on disk, not as raw code. We therefore create a temporary file for each page's proxy module, which we then delete as soon as rollup has read it. The easiest way to make sure that relative paths are preserved when things are re-exported is to put each temporary file alongside the page module it's wrapping, in the `pages/` directory. Fortunately, by the time our loader is running, Webpack has already decided what files it needs to load, so these temporary files don't get caught up in the bundling process. - In order to satisfy the linter in the template file, the SDK itself has been added as a dev dependency. Fortunately this seems not to confuse yarn. - Just to keep things manageable, this stops using but doesn't yet delete the previous loader (and associated files/code). Once this is merged, I'll do that in a separate PR. Ref #5505
1 parent 0bb0092 commit f7241c4

File tree

11 files changed

+326
-12
lines changed

11 files changed

+326
-12
lines changed

packages/nextjs/package.json

+3
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
},
1919
"dependencies": {
2020
"@babel/parser": "^7.18.10",
21+
"@rollup/plugin-sucrase": "4.0.4",
2122
"@sentry/core": "7.11.1",
2223
"@sentry/hub": "7.11.1",
2324
"@sentry/integrations": "7.11.1",
@@ -28,10 +29,12 @@
2829
"@sentry/utils": "7.11.1",
2930
"@sentry/webpack-plugin": "1.19.0",
3031
"jscodeshift": "^0.13.1",
32+
"rollup": "2.78.0",
3133
"tslib": "^1.9.3"
3234
},
3335
"devDependencies": {
3436
"@babel/types": "7.18.10",
37+
"@sentry/nextjs": "7.11.1",
3538
"@types/jscodeshift": "^0.11.5",
3639
"@types/webpack": "^4.41.31",
3740
"next": "10.1.3"

packages/nextjs/rollup.npm.config.js

+11-3
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index.js';
1+
import { makeBaseNPMConfig, makeNPMConfigVariants, plugins } from '../../rollup/index.js';
22

33
export default [
44
...makeNPMConfigVariants(
@@ -16,10 +16,12 @@ export default [
1616
makeBaseNPMConfig({
1717
entrypoints: [
1818
'src/config/templates/prefixLoaderTemplate.ts',
19+
'src/config/templates/proxyLoaderTemplate.ts',
1920
'src/config/templates/dataFetchersLoaderTemplate.ts',
2021
],
2122

2223
packageSpecificConfig: {
24+
plugins: [plugins.makeRemoveMultiLineCommentsPlugin()],
2325
output: {
2426
// Preserve the original file structure (i.e., so that everything is still relative to `src`). (Not entirely
2527
// clear why this is necessary here and not for other entrypoints in this file.)
@@ -29,20 +31,26 @@ export default [
2931
// shouldn't have them, lest they muck with the module to which we're adding it)
3032
sourcemap: false,
3133
esModule: false,
34+
35+
// make it so Rollup calms down about the fact that we're combining default and named exports
36+
exports: 'named',
3237
},
33-
external: ['@sentry/nextjs'],
38+
external: ['@sentry/nextjs', '__RESOURCE_PATH__'],
3439
},
3540
}),
3641
),
3742
...makeNPMConfigVariants(
3843
makeBaseNPMConfig({
3944
entrypoints: ['src/config/loaders/index.ts'],
45+
// Needed in order to successfully import sucrase
46+
esModuleInterop: true,
4047

4148
packageSpecificConfig: {
4249
output: {
43-
// make it so Rollup calms down about the fact that we're doing `export { loader as default }`
50+
// make it so Rollup calms down about the fact that we're combining default and named exports
4451
exports: 'named',
4552
},
53+
external: ['@rollup/plugin-sucrase', 'rollup'],
4654
},
4755
}),
4856
),
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
11
export { default as prefixLoader } from './prefixLoader';
22
export { default as dataFetchersLoader } from './dataFetchersLoader';
3+
export { default as proxyLoader } from './proxyLoader';
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
import { escapeStringForRegex } from '@sentry/utils';
2+
import * as fs from 'fs';
3+
import * as path from 'path';
4+
5+
import { rollupize } from './rollup';
6+
import { LoaderThis } from './types';
7+
8+
type LoaderOptions = {
9+
pagesDir: string;
10+
};
11+
12+
/**
13+
* Replace the loaded file with a proxy module "wrapping" the original file. In the proxy, the original file is loaded,
14+
* any data-fetching functions (`getInitialProps`, `getStaticProps`, and `getServerSideProps`) it contains are wrapped,
15+
* and then everything is re-exported.
16+
*/
17+
export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userCode: string): Promise<string> {
18+
// We know one or the other will be defined, depending on the version of webpack being used
19+
const { pagesDir } = 'getOptions' in this ? this.getOptions() : this.query;
20+
21+
// Get the parameterized route name from this page's filepath
22+
const parameterizedRoute = path
23+
// Get the path of the file insde of the pages directory
24+
.relative(pagesDir, this.resourcePath)
25+
// Add a slash at the beginning
26+
.replace(/(.*)/, '/$1')
27+
// Pull off the file extension
28+
.replace(/\.(jsx?|tsx?)/, '')
29+
// Any page file named `index` corresponds to root of the directory its in, URL-wise, so turn `/xyz/index` into
30+
// just `/xyz`
31+
.replace(/\/index$/, '')
32+
// In case all of the above have left us with an empty string (which will happen if we're dealing with the
33+
// homepage), sub back in the root route
34+
.replace(/^$/, '/');
35+
36+
// TODO: For the moment we skip API routes. Those will need to be handled slightly differently because of the manual
37+
// wrapping we've already been having people do using `withSentry`.
38+
if (parameterizedRoute.startsWith('api')) {
39+
return userCode;
40+
}
41+
42+
// We don't want to wrap twice (or infinitely), so in the proxy we add this query string onto references to the
43+
// wrapped file, so that we know that it's already been processed. (Adding this query string is also necessary to
44+
// convince webpack that it's a different file than the one it's in the middle of loading now, so that the originals
45+
// themselves will have a chance to load.)
46+
if (this.resourceQuery.includes('__sentry_wrapped__')) {
47+
return userCode;
48+
}
49+
50+
const templatePath = path.resolve(__dirname, '../templates/proxyLoaderTemplate.js');
51+
let templateCode = fs.readFileSync(templatePath).toString();
52+
// Make sure the template is included when runing `webpack watch`
53+
this.addDependency(templatePath);
54+
55+
// Inject the route into the template
56+
templateCode = templateCode.replace(/__ROUTE__/g, parameterizedRoute);
57+
58+
// Fill in the path to the file we're wrapping and save the result as a temporary file in the same folder (so that
59+
// relative imports and exports are calculated correctly).
60+
//
61+
// TODO: We're saving the filled-in template to disk, however temporarily, because Rollup expects a path to a code
62+
// file, not code itself. There is a rollup plugin which can fake this (`@rollup/plugin-virtual`) but the virtual file
63+
// seems to be inside of a virtual directory (in other words, one level down from where you'd expect it) and that
64+
// messes up relative imports and exports. Presumably there's a way to make it work, though, and if we can, it would
65+
// be cleaner than having to first write and then delete a temporary file each time we run this loader.
66+
templateCode = templateCode.replace(/__RESOURCE_PATH__/g, this.resourcePath);
67+
const tempFilePath = path.resolve(path.dirname(this.resourcePath), `temp${Math.random()}.js`);
68+
fs.writeFileSync(tempFilePath, templateCode);
69+
70+
// Run the proxy module code through Rollup, in order to split the `export * from '<wrapped file>'` out into
71+
// individual exports (which nextjs seems to require), then delete the tempoary file.
72+
let proxyCode = await rollupize(tempFilePath, this.resourcePath);
73+
fs.unlinkSync(tempFilePath);
74+
75+
if (!proxyCode) {
76+
// We will already have thrown a warning in `rollupize`, so no need to do it again here
77+
return userCode;
78+
}
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;
91+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
import type { RollupSucraseOptions } from '@rollup/plugin-sucrase';
2+
import sucrase from '@rollup/plugin-sucrase';
3+
import { logger } from '@sentry/utils';
4+
import * as path from 'path';
5+
import type { InputOptions as RollupInputOptions, OutputOptions as RollupOutputOptions } from 'rollup';
6+
import { rollup } from 'rollup';
7+
8+
const getRollupInputOptions: (proxyPath: string, resourcePath: string) => RollupInputOptions = (
9+
proxyPath,
10+
resourcePath,
11+
) => ({
12+
input: proxyPath,
13+
plugins: [
14+
// For some reason, even though everything in `RollupSucraseOptions` besides `transforms` is supposed to be
15+
// optional, TS complains that there are a bunch of missing properties (hence the typecast). Similar to
16+
// https://github.com/microsoft/TypeScript/issues/20722, though that's been fixed. (In this case it's an interface
17+
// exporting a `Pick` picking optional properties which is turning them required somehow.)'
18+
sucrase({
19+
transforms: ['jsx', 'typescript'],
20+
} as unknown as RollupSucraseOptions),
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 !== proxyPath && importPath !== resourcePath,
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+
// It's not 100% clear why, but telling it not to do the conversion back from absolute to relative (by setting
52+
// `makeAbsoluteExternalsRelative` to `false`) seems to also prevent it from going from relative to absolute in the
53+
// first place, with the result that the path remains untouched (which is what we want.)
54+
makeAbsoluteExternalsRelative: false,
55+
});
56+
57+
const rollupOutputOptions: RollupOutputOptions = {
58+
format: 'esm',
59+
60+
// Don't create a bundle - we just want the transformed entrypoint file
61+
preserveModules: true,
62+
};
63+
64+
/**
65+
* Use Rollup to process the proxy module file (located at `tempProxyFilePath`) in order to split its `export * from
66+
* '<wrapped file>'` call into individual exports (which nextjs seems to need).
67+
*
68+
* @param tempProxyFilePath The path to the temporary file containing the proxy module code
69+
* @param resourcePath The path to the file being wrapped
70+
* @returns The processed proxy module code, or undefined if an error occurs
71+
*/
72+
export async function rollupize(tempProxyFilePath: string, resourcePath: string): Promise<string | undefined> {
73+
let finalBundle;
74+
75+
try {
76+
const intermediateBundle = await rollup(getRollupInputOptions(tempProxyFilePath, resourcePath));
77+
finalBundle = await intermediateBundle.generate(rollupOutputOptions);
78+
} catch (err) {
79+
__DEBUG_BUILD__ &&
80+
logger.warn(
81+
`Could not wrap ${resourcePath}. An error occurred while processing the proxy module template:\n${err}`,
82+
);
83+
return undefined;
84+
}
85+
86+
// The module at index 0 is always the entrypoint, which in this case is the proxy module.
87+
let { code } = finalBundle.output[0];
88+
89+
// Rollup does a few things to the code we *don't* want. Undo those changes before returning the code.
90+
//
91+
// Nextjs uses square brackets surrounding a path segment to denote a parameter in the route, but Rollup turns those
92+
// square brackets into underscores. Further, Rollup adds file extensions to bare-path-type import and export sources.
93+
// Because it assumes that everything will have already been processed, it always uses `.js` as the added extension.
94+
// We need to restore the original name and extension so that Webpack will be able to find the wrapped file.
95+
const resourceFilename = path.basename(resourcePath);
96+
const mutatedResourceFilename = resourceFilename
97+
// `[\\[\\]]` is the character class containing `[` and `]`
98+
.replace(new RegExp('[\\[\\]]', 'g'), '_')
99+
.replace(/(jsx?|tsx?)$/, 'js');
100+
code = code.replace(new RegExp(mutatedResourceFilename, 'g'), resourceFilename);
101+
102+
return code;
103+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
/**
2+
* This file is a template for the code which will be substituted when our webpack loader handles non-API files in the
3+
* `pages/` directory.
4+
*
5+
* We use `__RESOURCE_PATH__` as a placeholder for the path to the file being wrapped. Because it's not a real package,
6+
* this causes both TS and ESLint to complain, hence the pragma comments below.
7+
*/
8+
9+
// @ts-ignore See above
10+
// eslint-disable-next-line import/no-unresolved
11+
import * as wrapee from '__RESOURCE_PATH__';
12+
import * as Sentry from '@sentry/nextjs';
13+
import type { GetServerSideProps, GetStaticProps, NextPage as NextPageComponent } from 'next';
14+
15+
type NextPageModule = {
16+
default: { getInitialProps?: NextPageComponent['getInitialProps'] };
17+
getStaticProps?: GetStaticProps;
18+
getServerSideProps?: GetServerSideProps;
19+
};
20+
21+
const userPageModule = wrapee as NextPageModule;
22+
23+
const pageComponent = userPageModule.default;
24+
25+
const origGetInitialProps = pageComponent.getInitialProps;
26+
const origGetStaticProps = userPageModule.getStaticProps;
27+
const origGetServerSideProps = userPageModule.getServerSideProps;
28+
29+
if (typeof origGetInitialProps === 'function') {
30+
pageComponent.getInitialProps = Sentry.withSentryGetInitialProps(
31+
origGetInitialProps,
32+
'__ROUTE__',
33+
) as NextPageComponent['getInitialProps'];
34+
}
35+
36+
export const getStaticProps =
37+
typeof origGetStaticProps === 'function'
38+
? Sentry.withSentryGetStaticProps(origGetStaticProps, '__ROUTE__')
39+
: undefined;
40+
export const getServerSideProps =
41+
typeof origGetServerSideProps === 'function'
42+
? Sentry.withSentryGetServerSideProps(origGetServerSideProps, '__ROUTE__')
43+
: undefined;
44+
45+
export default pageComponent;
46+
47+
// Re-export anything exported by the page module we're wrapping. When processing this code, Rollup is smart enough to
48+
// not include anything whose name matchs something we've explicitly exported above.
49+
// @ts-ignore See above
50+
// eslint-disable-next-line import/no-unresolved
51+
export * from '__RESOURCE_PATH__';

packages/nextjs/src/config/webpack.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,8 @@ export function constructWebpackConfigFunction(
8686
test: new RegExp(`${escapeStringForRegex(projectDir)}(/src)?/pages/.*\\.(jsx?|tsx?)`),
8787
use: [
8888
{
89-
loader: path.resolve(__dirname, 'loaders/dataFetchersLoader.js'),
90-
options: { projectDir, pagesDir },
89+
loader: path.resolve(__dirname, 'loaders/proxyLoader.js'),
90+
options: { pagesDir },
9191
},
9292
],
9393
});

packages/nextjs/test/run-integration-tests.sh

+3-2
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,9 @@ for NEXTJS_VERSION in 10 11 12; do
6161
else
6262
sed -i /"next.*latest"/s/latest/"${NEXTJS_VERSION}.x"/ package.json
6363
fi
64-
yarn --no-lockfile --silent >/dev/null 2>&1
64+
# We have to use `--ignore-engines` because sucrase claims to need Node 12, even though tests pass just fine on Node
65+
# 10
66+
yarn --no-lockfile --ignore-engines --silent >/dev/null 2>&1
6567
# if applicable, use local versions of `@sentry/cli` and/or `@sentry/webpack-plugin` (these commands no-op unless
6668
# LINKED_CLI_REPO and/or LINKED_PLUGIN_REPO are set)
6769
linkcli && linkplugin
@@ -90,7 +92,6 @@ for NEXTJS_VERSION in 10 11 12; do
9092
exit 0
9193
fi
9294

93-
9495
# next 10 defaults to webpack 4 and next 11 defaults to webpack 5, but each can use either based on settings
9596
if [ "$NEXTJS_VERSION" -eq "10" ]; then
9697
sed "s/%RUN_WEBPACK_5%/$RUN_WEBPACK_5/g" <next10.config.template >next.config.js

packages/nextjs/tsconfig.types.json

+4
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
{
22
"extends": "./tsconfig.json",
33

4+
// Some of the templates for code we inject into a user's app include an import from `@sentry/nextjs`. This makes
5+
// creating types for these template files a circular exercise, which causes `tsc` to crash. Fortunately, since the
6+
// templates aren't consumed as modules (they're essentially just text files which happen to contain code), we don't
7+
// actually need to create types for them.
48
"exclude": ["src/config/templates/*"],
59

610
"compilerOptions": {

rollup/plugins/npmPlugins.js

+17
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,23 @@ export function makeRemoveBlankLinesPlugin() {
127127
});
128128
}
129129

130+
/**
131+
* Create a plugin to strip multi-line comments from the output.
132+
*
133+
* @returns A `rollup-plugin-re` instance.
134+
*/
135+
export function makeRemoveMultiLineCommentsPlugin() {
136+
return regexReplace({
137+
patterns: [
138+
{
139+
// The `s` flag makes the dot match newlines
140+
test: /^\/\*.*\*\//gs,
141+
replace: '',
142+
},
143+
],
144+
});
145+
}
146+
130147
/**
131148
* Creates a plugin to replace all instances of "__DEBUG_BUILD__" with a safe statement that
132149
* a) evaluates to `true`

0 commit comments

Comments
 (0)