Skip to content

feat(nextjs): Sourcemaps for custom build directories #4003

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

Closed
wants to merge 12 commits into from
Closed
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export function includeDistDir(
);
// Keep the same object even if it's incorrect, so that the user can get a more precise error from sentry-cli
// Casting to `any` for TS not complaining about it being `unknown`
// eslint-disable-next-line @typescript-eslint/no-explicit-any
sourcesToInclude = usersInclude as any;
}
}
Expand Down
90 changes: 90 additions & 0 deletions packages/nextjs/src/config/webpack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as SentryWebpackPlugin from '@sentry/webpack-plugin';
import * as fs from 'fs';
import * as path from 'path';

import { PROJECT_BASEPATH } from '../index.server';
import {
BuildContext,
EntryPointValue,
Expand Down Expand Up @@ -49,6 +50,14 @@ export function constructWebpackConfigFunction(
newConfig = userNextConfig.webpack(newConfig, buildContext);
}

// If a user defines a custom build directory (`distDir`), we must update the `RewriteFrames` integration so that
// the paths of the source maps match. This is required to correctly display server stack traces.
// `distDir` is always defined in real life: either the user defines a value, or Next.js sets the default `.next`.
// The check is for different environments, such as in tests.
if (buildContext.isServer && buildContext.config.distDir) {
updateRewriteFramesBasepath(buildContext.dir, buildContext.config.distDir as string);
}

// 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 Down Expand Up @@ -108,6 +117,87 @@ export function constructWebpackConfigFunction(
return newWebpackFunction;
}

/**
* WARNING: don't modify the contents of this variable.
* The variable it refers to must be available in `index.server.ts`, and it gets
* overridden at build time. If it isn't, the build may break.
*/
export const BASEPATH_VARNAME = 'PROJECT_BASEPATH';

/**
* Updates the base path of the server's RewriteFrames integration to match the source
* map's paths with the given distribution directory.
*
* This action is required to view correct stack traces when using a custom
* distribution directory. The project root is required to build correct paths of the
* files that must be overwritten. This use case arises when you run the build step in
* a different directory from the one the project is; for example, in tests.
*
* Why this approach?
* There are two times to define the path: run time and build time. In any case,
* Next.js requires the user to set `distDir` in the options to compile the project.
* To get this path at run time and the SDK build it, users must also define the
* option in the SDK's server's configuration. However, to get the path at build time,
* users only need to set it once since the `withSentryConfig` wrapper can read the
* user's Next.js configuration. The SDK generates the `RewriteFrames` integration at
* initialization, meaning it's impossible to dynamically set the distribution directory
* path at build time if the option is only available at run time. To solve this issue,
* the SDK at build time rewrites the files where the default path of the integration is,
* so it's successfully generated at run time without additional configuration.
*
* The project root is
* @param projectRootDir Root directory of the Next.js project.
* @param distDir The distribution directory.
*/
function updateRewriteFramesBasepath(projectRootDir: string, distDir: string): void {
if (distDir === PROJECT_BASEPATH) return;
try {
// esm
setProjectBasepath(
path.join(projectRootDir, 'node_modules', '@sentry', 'nextjs', 'esm', 'index.server.js'),
'var ',
distDir,
);
// es5
setProjectBasepath(
path.join(projectRootDir, 'node_modules', '@sentry', 'nextjs', 'dist', 'index.server.js'),
'exports.',
distDir,
);
} catch (error) {
// eslint-disable-next-line no-console
console.warn(
'Sentry Logger [Warn]: ' +
`Could not set custom build directory \`${distDir}\`, required for correct display of source maps. `,
'In order to set your own `RewriteFrames`, visit\n' +
'https://docs.sentry.io/platforms/javascript/guides/nextjs/configuration/integrations/plugin/#rewriteframes\n',
error,
);
}
}

/**
* Overrides the base path variable with the distDir provided.
*
* Note that only replaces the first regex match.
* Different module formats have different variable prefixes and their output files
* are located in different paths.
*
* There's no error handling.
*
* @param filePath Path to the file.
* @param varPrefix Prefix of the variable declaration.
* @param distDir The distribution directory.
*/
function setProjectBasepath(filePath: string, varPrefix: string, distDir: string): void {
const fileContents = fs.readFileSync(filePath).toString();
Copy link
Member

Choose a reason for hiding this comment

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

has to be sync? we can't async await?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial approach and afaik we can definitely do it, but since we're running the code before webpack runs, the number of files is small (2 currently, and this is not something that scales), and files themselves are small, I believe the sync approach is faster (even if we block the event loop). That said, if you think the async approach is more convenient, I'm happy to change it. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer defaulting to async, but I'll leave it up to you to make the final call.

const replacedContents = fileContents.replace(
new RegExp(`${varPrefix}${BASEPATH_VARNAME} = .*`),
`${varPrefix}${BASEPATH_VARNAME} = '${distDir}';`,
);
fs.writeFileSync(filePath, replacedContents);
}

/**
* Modify the webpack `entry` property so that the code in `sentry.server.config.js` and `sentry.client.config.js` is
* included in the the necessary bundles.
Expand Down
11 changes: 9 additions & 2 deletions packages/nextjs/src/index.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,18 @@ function sdkAlreadyInitialized(): boolean {
return !!hub.getClient();
}

const SOURCEMAP_FILENAME_REGEX = /^.*\/\.next\//;
/**
* WARNING: don't refactor this variable.
* This variable gets overridden at build time to set the correct path -- what users
* defined in the `distDir` option in their Next.js configuration.
*/
export const PROJECT_BASEPATH = '.next';
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should just go in it's own file - and then we can even snapshot test (https://jestjs.io/docs/snapshot-testing) against potential regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, yes. There's more code related to integrations that should be moved to another file, like the RewriteFrames integration itself and the addServerIntegrations below, and also the new code in the webpack file. However, that refactor isn't part of this PR and should go in another one.

const projectBasepathRegex = PROJECT_BASEPATH[0] === '.' ? `\\${PROJECT_BASEPATH}` : PROJECT_BASEPATH;
const sourcemapFilenameRegex = new RegExp(`^.*/${projectBasepathRegex}/`);

const defaultRewriteFramesIntegration = new RewriteFrames({
iteratee: frame => {
frame.filename = frame.filename?.replace(SOURCEMAP_FILENAME_REGEX, 'app:///_next/');
frame.filename = frame.filename?.replace(sourcemapFilenameRegex, 'app:///_next/');
return frame;
},
});
Expand Down
89 changes: 88 additions & 1 deletion packages/nextjs/test/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
WebpackConfigObject,
} from '../src/config/types';
import {
BASEPATH_VARNAME,
constructWebpackConfigFunction,
getUserConfigFile,
getWebpackPluginOptions,
Expand Down Expand Up @@ -103,7 +104,7 @@ function getBuildContext(
dev: false,
buildId: 'sItStAyLiEdOwN',
dir: '/Users/Maisey/projects/squirrelChasingSimulator',
config: { target: 'server', ...userNextConfig },
config: { target: 'server', distDir: '.next', ...userNextConfig },
webpack: { version: webpackVersion },
isServer: buildTarget === 'server',
};
Expand Down Expand Up @@ -324,6 +325,92 @@ describe('webpack config', () => {
);
});
});

describe('RewriteFrames on base path change', () => {
const testProjectBasepath = path.join(__dirname, 'testDir');
const testFilesPath = path.join(testProjectBasepath, 'node_modules', '@sentry', 'nextjs');

const moduleFormatExample = [
{
name: 'esm',
dir: 'esm',
file: 'index.server.js',
contents: "var PROJECT_BASEPATH = '.next'",
},
{
name: 'es5',
dir: 'dist',
file: 'index.server.js',
contents: "exports.PROJECT_BASEPATH = '.next'",
},
];

beforeAll(async () => {
await fs.promises.mkdir(testFilesPath, { recursive: true });
moduleFormatExample.map(format => {
format.dir = path.join(testFilesPath, format.dir);
format.file = path.join(format.dir, format.file);
return fs.promises.mkdir(format.dir);
});
});

afterAll(() => rimraf.sync(testProjectBasepath));

beforeEach(() =>
moduleFormatExample.map(
// Using promises here make reading the contents of the files to be empty strings
format => fs.writeFileSync(format.file, format.contents),
),
);

function addDistDirToBuildContext(buildCtxt: BuildContext, distDir: string): BuildContext {
const config = { ...buildCtxt.config, distDir };
const res = {
...buildCtxt,
config,
};
return res;
}

test.each([
['client without setting a dir', clientBuildContext, null],
['client setting a normal dir', addDistDirToBuildContext(clientBuildContext, 'test'), null],
['server without setting a dir', serverBuildContext, null],
[
'server setting normal dir',
addDistDirToBuildContext(serverBuildContext, 'test'),
{
esm: "var PROJECT_BASEPATH = 'test';",
es5: "exports.PROJECT_BASEPATH = 'test';",
},
],
[
'server setting hidden dir',
addDistDirToBuildContext(serverBuildContext, '.test'),
{
esm: "var PROJECT_BASEPATH = '.test';",
es5: "exports.PROJECT_BASEPATH = '.test';",
},
],
// `expectedContents === null` => contents shouldn't change
])('%s', async (_testName, buildContext, expectedContents: null | Record<string, string>) => {
await materializeFinalWebpackConfig({
userNextConfig,
incomingWebpackConfig: clientWebpackConfig,
incomingWebpackBuildContext: { ...buildContext, dir: testProjectBasepath },
});
moduleFormatExample.map(format => {
const contents = fs.readFileSync(format.file).toString();
expect(contents).toStrictEqual(expectedContents ? expectedContents[format.name] : format.contents);
});
});

test(`\`${BASEPATH_VARNAME}\` is defined in \`index.server.ts\``, () => {
const indexServerPath = path.join(__dirname, '..', 'src', 'index.server.ts');
const fileContents = fs.readFileSync(indexServerPath).toString();
expect(fileContents).toMatch(new RegExp(`\\Wconst ${BASEPATH_VARNAME} =\\W`));
});
});
});

describe('Sentry webpack plugin config', () => {
Expand Down