Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(nextjs): Use proxy loader for wrapping all data-fetching functions #5602

Merged
merged 5 commits into from
Aug 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/nextjs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
},
"dependencies": {
"@babel/parser": "^7.18.10",
"@rollup/plugin-sucrase": "4.0.4",
"@sentry/core": "7.11.1",
"@sentry/hub": "7.11.1",
"@sentry/integrations": "7.11.1",
Expand All @@ -28,10 +29,12 @@
"@sentry/utils": "7.11.1",
"@sentry/webpack-plugin": "1.19.0",
"jscodeshift": "^0.13.1",
"rollup": "2.78.0",
"tslib": "^1.9.3"
},
"devDependencies": {
"@babel/types": "7.18.10",
"@sentry/nextjs": "7.11.1",
"@types/jscodeshift": "^0.11.5",
"@types/webpack": "^4.41.31",
"next": "10.1.3"
Expand Down
14 changes: 11 additions & 3 deletions packages/nextjs/rollup.npm.config.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index.js';
import { makeBaseNPMConfig, makeNPMConfigVariants, plugins } from '../../rollup/index.js';

export default [
...makeNPMConfigVariants(
Expand All @@ -16,10 +16,12 @@ export default [
makeBaseNPMConfig({
entrypoints: [
'src/config/templates/prefixLoaderTemplate.ts',
'src/config/templates/proxyLoaderTemplate.ts',
'src/config/templates/dataFetchersLoaderTemplate.ts',
],

packageSpecificConfig: {
plugins: [plugins.makeRemoveMultiLineCommentsPlugin()],
output: {
// Preserve the original file structure (i.e., so that everything is still relative to `src`). (Not entirely
// clear why this is necessary here and not for other entrypoints in this file.)
Expand All @@ -29,20 +31,26 @@ export default [
// shouldn't have them, lest they muck with the module to which we're adding it)
sourcemap: false,
esModule: false,

// make it so Rollup calms down about the fact that we're combining default and named exports
exports: 'named',
},
external: ['@sentry/nextjs'],
external: ['@sentry/nextjs', '__RESOURCE_PATH__'],
},
}),
),
...makeNPMConfigVariants(
makeBaseNPMConfig({
entrypoints: ['src/config/loaders/index.ts'],
// Needed in order to successfully import sucrase
esModuleInterop: true,

packageSpecificConfig: {
output: {
// make it so Rollup calms down about the fact that we're doing `export { loader as default }`
// make it so Rollup calms down about the fact that we're combining default and named exports
exports: 'named',
},
external: ['@rollup/plugin-sucrase', 'rollup'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a thought. Considering we depend on a very specific behaviour of rollup (i.e. the unwrapping of the * in export * from ...), what do you think about doing the following two things:

  • We pin the exact version of rollup we're using so this behaviour doesn't just randomly break with an update.
  • We bundle rollup within our loader so that pinning the version doesn't turn into a dependency fiasco for our downstream users.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Pinning I think is a good idea (which is why I did it! 😛)

  2. You mean like vendoring the code ourselves? What kind of fiasco are you picturing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Sorry I am dumb. Sometimes im just writing down thoughts as suggestions without checking if they arent already there. Good that we're on the same page there.
  2. I am double dumb. The dep problem i was thinking of is with peer deps and not normal deps. I think we're good here!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL. Terrific. Now we're even for the other day when I entirely forgot the wrappers existed for about five minutes. And I'm the one who originally wrote them!

},
}),
),
Expand Down
1 change: 1 addition & 0 deletions packages/nextjs/src/config/loaders/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export { default as prefixLoader } from './prefixLoader';
export { default as dataFetchersLoader } from './dataFetchersLoader';
export { default as proxyLoader } from './proxyLoader';
91 changes: 91 additions & 0 deletions packages/nextjs/src/config/loaders/proxyLoader.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { escapeStringForRegex } from '@sentry/utils';
import * as fs from 'fs';
import * as path from 'path';

import { rollupize } from './rollup';
import { LoaderThis } from './types';

type LoaderOptions = {
pagesDir: string;
};

/**
* Replace the loaded file with a proxy module "wrapping" the original file. In the proxy, the original file is loaded,
* any data-fetching functions (`getInitialProps`, `getStaticProps`, and `getServerSideProps`) it contains are wrapped,
* and then everything is re-exported.
*/
export default async function proxyLoader(this: LoaderThis<LoaderOptions>, userCode: string): Promise<string> {
// We know one or the other will be defined, depending on the version of webpack being used
const { pagesDir } = 'getOptions' in this ? this.getOptions() : this.query;

// Get the parameterized route name from this page's filepath
const parameterizedRoute = path
// Get the path of the file insde of the pages directory
.relative(pagesDir, this.resourcePath)
// Add a slash at the beginning
.replace(/(.*)/, '/$1')
// Pull off the file extension
.replace(/\.(jsx?|tsx?)/, '')
// Any page file named `index` corresponds to root of the directory its in, URL-wise, so turn `/xyz/index` into
// just `/xyz`
.replace(/\/index$/, '')
// In case all of the above have left us with an empty string (which will happen if we're dealing with the
// homepage), sub back in the root route
.replace(/^$/, '/');

// TODO: For the moment we skip API routes. Those will need to be handled slightly differently because of the manual
// wrapping we've already been having people do using `withSentry`.
if (parameterizedRoute.startsWith('api')) {
return userCode;
}

// We don't want to wrap twice (or infinitely), so in the proxy we add this query string onto references to the
// 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__')) {
return userCode;
}

const templatePath = path.resolve(__dirname, '../templates/proxyLoaderTemplate.js');
let templateCode = fs.readFileSync(templatePath).toString();
// Make sure the template is included when runing `webpack watch`
this.addDependency(templatePath);

// Inject the route into the template
templateCode = templateCode.replace(/__ROUTE__/g, parameterizedRoute);

// 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.
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
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(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);
proxyCode = proxyCode.replace(
new RegExp(`/${escapeStringForRegex(resourceFilename)}'`, 'g'),
`/${resourceFilename}?__sentry_wrapped__'`,
);

return proxyCode;
}
103 changes: 103 additions & 0 deletions packages/nextjs/src/config/loaders/rollup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import type { RollupSucraseOptions } from '@rollup/plugin-sucrase';
import sucrase from '@rollup/plugin-sucrase';
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 getRollupInputOptions: (proxyPath: string, resourcePath: string) => RollupInputOptions = (
proxyPath,
resourcePath,
) => ({
input: proxyPath,
plugins: [
// 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 !== 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
// https://stackoverflow.com/a/60347490.)
context: 'this',

// Rollup's path-resolution logic when handling re-exports can go wrong when wrapping pages which aren't at the root
// level of the `pages` directory. This may be a bug, as it doesn't match the behavior described in the docs, but what
// seems to happen is this:
//
// - We try to wrap `pages/xyz/userPage.js`, which contains `export { helperFunc } from '../../utils/helper'`
// - Rollup converts '../../utils/helper' into an absolute path
// - We mark the helper module as external
// - Rollup then converts it back to a relative path, but relative to `pages/` rather than `pages/xyz/`. (This is
// the part which doesn't match the docs. They say that Rollup will use the common ancestor of all modules in the
// bundle as the basis for the relative path calculation, but both our temporary file and the page being wrapped
// live in `pages/xyz/`, and they're the only two files in the bundle, so `pages/xyz/`` should be used as the
// root. Unclear why it's not.)
// - As a result of the miscalculation, our proxy module will include `export { helperFunc } from '../utils/helper'`
// rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in
// nextjs..
//
// It's not 100% clear why, but telling it not to do the conversion back from absolute to relative (by setting
// `makeAbsoluteExternalsRelative` to `false`) seems to also prevent it from going from relative to absolute in the
// first place, with the result that the path remains untouched (which is what we want.)
makeAbsoluteExternalsRelative: false,
});

const rollupOutputOptions: RollupOutputOptions = {
format: 'esm',

// Don't create a bundle - we just want the transformed entrypoint file
preserveModules: true,
};

/**
* Use Rollup to process the proxy module file (located at `tempProxyFilePath`) in order to split its `export * from
* '<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 resourcePath The path to the file being wrapped
* @returns The processed proxy module code, or undefined if an error occurs
*/
export async function rollupize(tempProxyFilePath: string, resourcePath: string): Promise<string | undefined> {
let finalBundle;

try {
const intermediateBundle = await rollup(getRollupInputOptions(tempProxyFilePath, resourcePath));
finalBundle = await intermediateBundle.generate(rollupOutputOptions);
} catch (err) {
__DEBUG_BUILD__ &&
logger.warn(
`Could not wrap ${resourcePath}. An error occurred while processing the proxy module template:\n${err}`,
);
return undefined;
}

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

// Rollup does a few things to the code we *don't* want. Undo those changes before returning the code.
//
// Nextjs uses square brackets surrounding a path segment to denote a parameter in the route, but Rollup turns those
// 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(resourcePath);
const mutatedResourceFilename = resourceFilename
// `[\\[\\]]` is the character class containing `[` and `]`
.replace(new RegExp('[\\[\\]]', 'g'), '_')
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
.replace(/(jsx?|tsx?)$/, 'js');
code = code.replace(new RegExp(mutatedResourceFilename, 'g'), resourceFilename);

return code;
}
51 changes: 51 additions & 0 deletions packages/nextjs/src/config/templates/proxyLoaderTemplate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/**
* This file is a template for the code which will be substituted when our webpack loader handles non-API files in the
* `pages/` directory.
*
* 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.
*/

// @ts-ignore See above
// eslint-disable-next-line import/no-unresolved
import * as wrapee from '__RESOURCE_PATH__';
import * as Sentry from '@sentry/nextjs';
import type { GetServerSideProps, GetStaticProps, NextPage as NextPageComponent } from 'next';

type NextPageModule = {
default: { getInitialProps?: NextPageComponent['getInitialProps'] };
getStaticProps?: GetStaticProps;
getServerSideProps?: GetServerSideProps;
};

const userPageModule = wrapee as NextPageModule;

const pageComponent = userPageModule.default;

const origGetInitialProps = pageComponent.getInitialProps;
const origGetStaticProps = userPageModule.getStaticProps;
const origGetServerSideProps = userPageModule.getServerSideProps;

if (typeof origGetInitialProps === 'function') {
pageComponent.getInitialProps = Sentry.withSentryGetInitialProps(
origGetInitialProps,
'__ROUTE__',
) as NextPageComponent['getInitialProps'];
}

export const getStaticProps =
typeof origGetStaticProps === 'function'
? Sentry.withSentryGetStaticProps(origGetStaticProps, '__ROUTE__')
: undefined;
export const getServerSideProps =
typeof origGetServerSideProps === 'function'
? Sentry.withSentryGetServerSideProps(origGetServerSideProps, '__ROUTE__')
: undefined;

export default pageComponent;

// Re-export anything exported by the page module we're wrapping. When processing this code, Rollup is smart enough to
// not include anything whose name matchs something we've explicitly exported above.
// @ts-ignore See above
// eslint-disable-next-line import/no-unresolved
export * from '__RESOURCE_PATH__';
4 changes: 2 additions & 2 deletions packages/nextjs/src/config/webpack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ export function constructWebpackConfigFunction(
test: new RegExp(`${escapeStringForRegex(projectDir)}(/src)?/pages/.*\\.(jsx?|tsx?)`),
use: [
{
loader: path.resolve(__dirname, 'loaders/dataFetchersLoader.js'),
options: { projectDir, pagesDir },
loader: path.resolve(__dirname, 'loaders/proxyLoader.js'),
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
options: { pagesDir },
},
],
});
Expand Down
5 changes: 3 additions & 2 deletions packages/nextjs/test/run-integration-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ for NEXTJS_VERSION in 10 11 12; do
else
sed -i /"next.*latest"/s/latest/"${NEXTJS_VERSION}.x"/ package.json
fi
yarn --no-lockfile --silent >/dev/null 2>&1
# We have to use `--ignore-engines` because sucrase claims to need Node 12, even though tests pass just fine on Node
# 10
yarn --no-lockfile --ignore-engines --silent >/dev/null 2>&1
# if applicable, use local versions of `@sentry/cli` and/or `@sentry/webpack-plugin` (these commands no-op unless
# LINKED_CLI_REPO and/or LINKED_PLUGIN_REPO are set)
linkcli && linkplugin
Expand Down Expand Up @@ -90,7 +92,6 @@ for NEXTJS_VERSION in 10 11 12; do
exit 0
fi


# next 10 defaults to webpack 4 and next 11 defaults to webpack 5, but each can use either based on settings
if [ "$NEXTJS_VERSION" -eq "10" ]; then
sed "s/%RUN_WEBPACK_5%/$RUN_WEBPACK_5/g" <next10.config.template >next.config.js
Expand Down
4 changes: 4 additions & 0 deletions packages/nextjs/tsconfig.types.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
{
"extends": "./tsconfig.json",

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

"compilerOptions": {
Expand Down
17 changes: 17 additions & 0 deletions rollup/plugins/npmPlugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,23 @@ export function makeRemoveBlankLinesPlugin() {
});
}

/**
* Create a plugin to strip multi-line comments from the output.
*
* @returns A `rollup-plugin-re` instance.
*/
export function makeRemoveMultiLineCommentsPlugin() {
return regexReplace({
patterns: [
{
// The `s` flag makes the dot match newlines
test: /^\/\*.*\*\//gs,
replace: '',
},
],
});
}

/**
* Creates a plugin to replace all instances of "__DEBUG_BUILD__" with a safe statement that
* a) evaluates to `true`
Expand Down
Loading