Skip to content

Commit

Permalink
ref(nextjs): Use loader to set RewriteFrames helper value (#5445)
Browse files Browse the repository at this point in the history
In order to work, in the nextjs SDK our server-side `RewriteFrames` integration needs to know the name of the directory into which the app is built. While that information is readily available at build time, it isn't at runtime, so we adjust the webpack config to inject a global variable with the correct value in before the the injected `Sentry.init()` call from `sentry.server.config.js` wherever we inject that `Sentry.init().

Currently, this is done by creating a temporary file with the relevant code and adding it to the relevant pages' `entry` value along with `sentry.server.config.js`. This works, but has its downsides[1], and is a fair amount of machinery just to add a single line of code.

This injects the same line of code using a loader, which is really just a transformation function for the code from matching files. In this case, we're modifying `sentry.server.config.js` itself, so that by the time it's added to various pages' entry points, it already contains the relevant code. Since we don't know what the value will be ahead of time, there's a template, which the loader then populates before injecting the new code.

 _But how is that any less machinery?_, you might ask. _All of that, still for just one line of code?_ Honestly, it's not. But in the quest to improve parameterization of transactions names, we're going to be adding a loader in any case. Adding the `RewriteFrames` value thus provides the perfect proof of concept, precisely because it's so simple, letting us get the loader working separate from all of the other changes that work will entail.

Notes:

- I moved setting the default value for `distDir` from retrieval-of-the-value time to storage-of-the-value time, because using a loader necessarily stringifies everything, meaning `undefined` was turning into the string `'undefined'`, preventing the default from getting picked up.
- There are a few scattered lines of unrelated changes, artifacts of the fact that I ended up unpacking a few values at the top of the new webpack function.

[1] #4813
  • Loading branch information
lobsterkatie authored Jul 25, 2022
1 parent fc7682c commit 57c964a
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 81 deletions.
55 changes: 45 additions & 10 deletions packages/nextjs/rollup.npm.config.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,47 @@
import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index.js';

export default makeNPMConfigVariants(
makeBaseNPMConfig({
// We need to include `instrumentServer.ts` separately because it's only conditionally required, and so rollup
// doesn't automatically include it when calculating the module dependency tree.
entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/utils/instrumentServer.ts'],
// prevent this nextjs code from ending up in our built package (this doesn't happen automatially because the name
// doesn't match an SDK dependency)
packageSpecificConfig: { external: ['next/router'] },
}),
);
export default [
...makeNPMConfigVariants(
makeBaseNPMConfig({
// We need to include `instrumentServer.ts` separately because it's only conditionally required, and so rollup
// doesn't automatically include it when calculating the module dependency tree.
entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/utils/instrumentServer.ts'],

// prevent this internal nextjs code from ending up in our built package (this doesn't happen automatially because
// the name doesn't match an SDK dependency)
packageSpecificConfig: { external: ['next/router'] },
}),
),
...makeNPMConfigVariants(
makeBaseNPMConfig({
entrypoints: ['src/config/prefixLoaderTemplate.ts'],

packageSpecificConfig: {
output: {
// preserve the original file structure (i.e., so that everything is still relative to `src`)
entryFileNames: 'config/[name].js',

// this is going to be add-on code, so it doesn't need the trappings of a full module (and in fact actively
// shouldn't have them, lest they muck with the module to which we're adding it)
sourcemap: false,
esModule: false,
},
},
}),
),
...makeNPMConfigVariants(
makeBaseNPMConfig({
entrypoints: ['src/config/prefixLoader.ts'],

packageSpecificConfig: {
output: {
// make it so Rollup calms down about the fact that we're doing `export { loader as default }`
exports: 'default',

// preserve the original file structure (i.e., so that everything is still relative to `src`)
entryFileNames: 'config/[name].js',
},
},
}),
),
];
34 changes: 34 additions & 0 deletions packages/nextjs/src/config/prefixLoader.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import * as fs from 'fs';
import * as path from 'path';

type LoaderOptions = {
distDir: string;
};
// TODO Use real webpack types
type LoaderThis = {
// Webpack 4
query?: LoaderOptions;
// Webpack 5
getOptions?: () => LoaderOptions;
addDependency: (filepath: string) => void;
};

/**
* Inject templated code into the beginning of a module.
*/
function prefixLoader(this: LoaderThis, userCode: string): string {
// We know one or the other will be defined, depending on the version of webpack being used
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const { distDir } = this.getOptions ? this.getOptions() : this.query!;

const templatePath = path.resolve(__dirname, 'prefixLoaderTemplate.js');
this.addDependency(templatePath);

// Fill in the placeholder
let templateCode = fs.readFileSync(templatePath).toString();
templateCode = templateCode.replace('__DIST_DIR__', distDir);

return `${templateCode}\n${userCode}`;
}

export { prefixLoader as default };
5 changes: 5 additions & 0 deletions packages/nextjs/src/config/prefixLoaderTemplate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
(global as any).__rewriteFramesDistDir__ = '__DIST_DIR__';

// We need this to make this file an ESM module, which TS requires when using `isolatedModules`.
export {};
9 changes: 9 additions & 0 deletions packages/nextjs/src/config/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,15 @@ export type WebpackConfigObject = {
resolve?: {
alias?: { [key: string]: string | boolean };
};
module?: {
rules: Array<{
test: string | RegExp;
use: Array<{
loader: string;
options: Record<string, unknown>;
}>;
}>;
};
} & {
// other webpack options
[key: string]: unknown;
Expand Down
49 changes: 28 additions & 21 deletions packages/nextjs/src/config/webpack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { getSentryRelease } from '@sentry/node';
import { dropUndefinedKeys, logger } from '@sentry/utils';
import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin';
import * as fs from 'fs';
import * as os from 'os';
import * as path from 'path';

import {
Expand Down Expand Up @@ -42,6 +41,7 @@ export function constructWebpackConfigFunction(
// we're building server or client, whether we're in dev, what version of webpack we're using, etc). Note that
// `incomingConfig` and `buildContext` are referred to as `config` and `options` in the nextjs docs.
const newWebpackFunction = (incomingConfig: WebpackConfigObject, buildContext: BuildContext): WebpackConfigObject => {
const { isServer, dev: isDev } = buildContext;
let newConfig = { ...incomingConfig };

// if user has custom webpack config (which always takes the form of a function), run it so we have actual values to
Expand All @@ -50,6 +50,29 @@ export function constructWebpackConfigFunction(
newConfig = userNextConfig.webpack(newConfig, buildContext);
}

if (isServer) {
newConfig.module = {
...newConfig.module,
rules: [
...(newConfig.module?.rules || []),
{
test: /sentry\.server\.config\.(jsx?|tsx?)/,
use: [
{
// Support non-default output directories by making the output path (easy to get here at build-time)
// available to the server SDK's default `RewriteFrames` instance (which needs it at runtime), by
// injecting code to attach it to `global`.
loader: path.resolve(__dirname, 'prefixLoader.js'),
options: {
distDir: userNextConfig.distDir || '.next',
},
},
],
},
],
};
}

// Tell webpack to inject user config files (containing the two `Sentry.init()` calls) into the appropriate output
// bundles. Store a separate reference to the original `entry` value to avoid an infinite loop. (If we don't do
// this, we'll have a statement of the form `x.y = () => f(x.y)`, where one of the things `f` does is call `x.y`.
Expand All @@ -70,7 +93,7 @@ export function constructWebpackConfigFunction(
// with the `--ignore-scripts` option, this will be blocked and the missing binary will cause an error when users
// try to build their apps.)
ensureCLIBinaryExists() &&
(buildContext.isServer
(isServer
? !userNextConfig.sentry?.disableServerWebpackPlugin
: !userNextConfig.sentry?.disableClientWebpackPlugin);

Expand All @@ -80,14 +103,13 @@ export function constructWebpackConfigFunction(

// Next doesn't let you change `devtool` in dev even if you want to, so don't bother trying - see
// https://github.com/vercel/next.js/blob/master/errors/improper-devtool.md
if (!buildContext.dev) {
if (!isDev) {
// `hidden-source-map` produces the same sourcemaps as `source-map`, but doesn't include the `sourceMappingURL`
// comment at the bottom. For folks who aren't publicly hosting their sourcemaps, this is helpful because then
// the browser won't look for them and throw errors into the console when it can't find them. Because this is a
// front-end-only problem, and because `sentry-cli` handles sourcemaps more reliably with the comment than
// without, the option to use `hidden-source-map` only applies to the client-side build.
newConfig.devtool =
userNextConfig.sentry?.hideSourceMaps && !buildContext.isServer ? 'hidden-source-map' : 'source-map';
newConfig.devtool = userNextConfig.sentry?.hideSourceMaps && !isServer ? 'hidden-source-map' : 'source-map';
}

newConfig.plugins = newConfig.plugins || [];
Expand Down Expand Up @@ -121,7 +143,7 @@ async function addSentryToEntryProperty(
// we know is that it won't have gotten *simpler* in form, so we only need to worry about the object and function
// options. See https://webpack.js.org/configuration/entry-context/#entry.

const { isServer, dir: projectDir, dev: isDev, config: userNextConfig } = buildContext;
const { isServer, dir: projectDir } = buildContext;

const newEntryProperty =
typeof currentEntryProperty === 'function' ? await currentEntryProperty() : { ...currentEntryProperty };
Expand All @@ -132,21 +154,6 @@ async function addSentryToEntryProperty(
// we need to turn the filename into a path so webpack can find it
const filesToInject = [`./${userConfigFile}`];

// Support non-default output directories by making the output path (easy to get here at build-time) available to the
// server SDK's default `RewriteFrames` instance (which needs it at runtime). Doesn't work when using the dev server
// because it somehow tricks the file watcher into thinking that compilation itself is a file change, triggering an
// infinite recompiling loop. (This should be fine because we don't upload sourcemaps in dev in any case.)
if (isServer && !isDev) {
const rewriteFramesHelper = path.resolve(
fs.mkdtempSync(path.resolve(os.tmpdir(), 'sentry-')),
'rewriteFramesHelper.js',
);
fs.writeFileSync(rewriteFramesHelper, `global.__rewriteFramesDistDir__ = '${userNextConfig.distDir}';\n`);
// stick our helper file ahead of the user's config file so the value is in the global namespace *before*
// `Sentry.init()` is called
filesToInject.unshift(rewriteFramesHelper);
}

// inject into all entry points which might contain user's code
for (const entryPointName in newEntryProperty) {
if (shouldAddSentryToEntryPoint(entryPointName, isServer)) {
Expand Down
2 changes: 1 addition & 1 deletion packages/nextjs/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ function sdkAlreadyInitialized(): boolean {

function addServerIntegrations(options: NextjsOptions): void {
// This value is injected at build time, based on the output directory specified in the build config
const distDirName = (global as GlobalWithDistDir).__rewriteFramesDistDir__ || '.next';
const distDirName = (global as GlobalWithDistDir).__rewriteFramesDistDir__;
// nextjs always puts the build directory at the project root level, which is also where you run `next start` from, so
// we can read in the project directory from the currently running process
const distDirAbsPath = path.resolve(process.cwd(), distDirName);
Expand Down
81 changes: 32 additions & 49 deletions packages/nextjs/test/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -332,35 +332,26 @@ describe('webpack config', () => {
incomingWebpackBuildContext: serverBuildContext,
});

const tempDir = mkdtempSyncSpy.mock.results[0].value;
const rewriteFramesHelper = path.join(tempDir, 'rewriteFramesHelper.js');

expect(finalWebpackConfig.entry).toEqual(
expect.objectContaining({
// original entrypoint value is a string
// (was 'private-next-pages/_error.js')
'pages/_error': [rewriteFramesHelper, serverConfigFilePath, 'private-next-pages/_error.js'],
'pages/_error': [serverConfigFilePath, 'private-next-pages/_error.js'],

// original entrypoint value is a string array
// (was ['./node_modules/smellOVision/index.js', 'private-next-pages/_app.js'])
'pages/_app': [
rewriteFramesHelper,
serverConfigFilePath,
'./node_modules/smellOVision/index.js',
'private-next-pages/_app.js',
],
'pages/_app': [serverConfigFilePath, './node_modules/smellOVision/index.js', 'private-next-pages/_app.js'],

// original entrypoint value is an object containing a string `import` value
// (was { import: 'private-next-pages/api/simulator/dogStats/[name].js' })
'pages/api/simulator/dogStats/[name]': {
import: [rewriteFramesHelper, serverConfigFilePath, 'private-next-pages/api/simulator/dogStats/[name].js'],
import: [serverConfigFilePath, 'private-next-pages/api/simulator/dogStats/[name].js'],
},

// original entrypoint value is an object containing a string array `import` value
// (was { import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/api/simulator/leaderboard.js'] })
'pages/api/simulator/leaderboard': {
import: [
rewriteFramesHelper,
serverConfigFilePath,
'./node_modules/dogPoints/converter.js',
'private-next-pages/api/simulator/leaderboard.js',
Expand All @@ -370,7 +361,7 @@ describe('webpack config', () => {
// original entrypoint value is an object containg properties besides `import`
// (was { import: 'private-next-pages/api/tricks/[trickName].js', dependOn: 'treats', })
'pages/api/tricks/[trickName]': {
import: [rewriteFramesHelper, serverConfigFilePath, 'private-next-pages/api/tricks/[trickName].js'],
import: [serverConfigFilePath, 'private-next-pages/api/tricks/[trickName].js'],
dependOn: 'treats', // untouched
},
}),
Expand Down Expand Up @@ -478,53 +469,45 @@ describe('webpack config', () => {
}),
);
});
});

it('does not inject `RewriteFrames` helper into client routes', async () => {
describe('webpack loaders', () => {
it('adds loader to server config', async () => {
const finalWebpackConfig = await materializeFinalWebpackConfig({
userNextConfig,
incomingWebpackConfig: serverWebpackConfig,
incomingWebpackBuildContext: serverBuildContext,
});

expect(finalWebpackConfig.module!.rules).toEqual(
expect.arrayContaining([
{
test: expect.any(RegExp),
use: [
{
loader: expect.any(String),
// Having no criteria for what the object contains is better than using `expect.any(Object)`, because that
// could be anything
options: expect.objectContaining({}),
},
],
},
]),
);
});

it("doesn't add loader to client config", async () => {
const finalWebpackConfig = await materializeFinalWebpackConfig({
userNextConfig,
incomingWebpackConfig: clientWebpackConfig,
incomingWebpackBuildContext: clientBuildContext,
});

expect(finalWebpackConfig.entry).toEqual(
expect.objectContaining({
// was 'next-client-pages-loader?page=%2F_app', and now has client config but not`RewriteFrames` helper injected
'pages/_app': [clientConfigFilePath, 'next-client-pages-loader?page=%2F_app'],
}),
);
expect(finalWebpackConfig.module).toBeUndefined();
});
});

describe('`distDir` value in default server-side `RewriteFrames` integration', () => {
it.each([
['no custom `distDir`', undefined, '.next'],
['custom `distDir`', 'dist', 'dist'],
])(
'creates file injecting `distDir` value into `global` - %s',
async (_name, customDistDir, expectedInjectedValue) => {
// Note: the fact that the file tested here gets injected correctly is covered in the 'webpack `entry` property
// config' tests above

const userNextConfigDistDir = {
...userNextConfig,
...(customDistDir && { distDir: customDistDir }),
};
await materializeFinalWebpackConfig({
userNextConfig: userNextConfigDistDir,
incomingWebpackConfig: serverWebpackConfig,
incomingWebpackBuildContext: getBuildContext('server', userNextConfigDistDir),
});

const tempDir = mkdtempSyncSpy.mock.results[0].value;
const rewriteFramesHelper = path.join(tempDir, 'rewriteFramesHelper.js');

expect(fs.existsSync(rewriteFramesHelper)).toBe(true);

const injectedCode = fs.readFileSync(rewriteFramesHelper).toString();
expect(injectedCode).toEqual(`global.__rewriteFramesDistDir__ = '${expectedInjectedValue}';\n`);
},
);

describe('`RewriteFrames` ends up with correct `distDir` value', () => {
// TODO: this, along with any number of other parts of the build process, should be tested with an integration
// test which actually runs webpack and inspects the resulting bundles (and that integration test should test
Expand Down

0 comments on commit 57c964a

Please sign in to comment.