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): Make build-phase check more robust #5857

Merged
merged 4 commits into from Oct 4, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 18 additions & 4 deletions packages/nextjs/src/index.server.ts
Expand Up @@ -22,7 +22,14 @@ export { ErrorBoundary, showReportDialog, withErrorBoundary } from '@sentry/reac
type GlobalWithDistDir = typeof global & { __rewriteFramesDistDir__: string };
const domain = domainModule as typeof domainModule & { active: (domainModule.Domain & Carrier) | null };

const isVercel = !!process.env.VERCEL;
// Exporting this constant means we can compute it without the linter complaining, even if we stop directly using it in
// this file. It's important that it be computed as early as possible, because one of its indicators is seeing 'build'
// (as in the CLI command `next build`) in `process.argv`. Later on in the build process, everything's been spun out
// into child threads and `argv` turns into ['node', 'path/to/childProcess.js'], so the original indicator is lost. We
// thus want to compute it as soon as the SDK is loaded for the first time, which is normally when the user imports
// `withSentryConfig` into `next.config.js`.
export const IS_BUILD = isBuild();
const IS_VERCEL = !!process.env.VERCEL;

/** Inits the Sentry NextJS SDK on node. */
export function init(options: NextjsOptions): void {
Expand Down Expand Up @@ -63,7 +70,7 @@ export function init(options: NextjsOptions): void {

configureScope(scope => {
scope.setTag('runtime', 'node');
if (isVercel) {
if (IS_VERCEL) {
scope.setTag('vercel', true);
}

Expand Down Expand Up @@ -124,9 +131,16 @@ function addServerIntegrations(options: NextjsOptions): void {
options.integrations = integrations;
}

// TODO (v8): Remove this
/**
* @deprecated Use the constant `IS_BUILD` instead.
*/
const deprecatedIsBuild = (): boolean => isBuild();
// eslint-disable-next-line deprecation/deprecation
export { deprecatedIsBuild as isBuild };
Copy link
Member

Choose a reason for hiding this comment

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

l: ehhh do we have to deprecate this? Is anyone even using this export?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea, but it seems to me we don't lose much by deprecating it.


export type { SentryWebpackPluginOptions } from './config/types';
export { withSentryConfig } from './config';
export { isBuild } from './utils/isBuild';
export {
withSentryGetServerSideProps,
withSentryGetStaticProps,
Expand All @@ -142,7 +156,7 @@ export {
// deployments, because the current method of doing the wrapping a) crashes Next 12 apps deployed to Vercel and
// b) doesn't work on those apps anyway. We also don't do it during build, because there's no server running in that
// phase.)
if (!isBuild() && !isVercel) {
if (!IS_BUILD && !IS_VERCEL) {
// Dynamically require the file because even importing from it causes Next 12 to crash on Vercel.
// In environments where the JS file doesn't exist, such as testing, import the TS file.
try {
Expand Down
30 changes: 18 additions & 12 deletions packages/nextjs/src/utils/isBuild.ts
Expand Up @@ -2,18 +2,24 @@
* Decide if the currently running process is part of the build phase or happening at runtime.
*/
export function isBuild(): boolean {
// During build, the main process is invoked by
// `node next build`
// and child processes are invoked as
// `node <path>/node_modules/.../jest-worker/processChild.js`.
// The former is (obviously) easy to recognize, but the latter could happen at runtime as well. Fortunately, the main
// process hits this file before any of the child processes do, so we're able to set an env variable which the child
// processes can then check. During runtime, the main process is invoked as
// `node next start`
// or
// `node /var/runtime/index.js`,
// so we never drop into the `if` in the first place.
if (process.argv.includes('build') || process.env.SENTRY_BUILD_PHASE) {
if (
// During build, the main process is invoked by
// `node next build`
// and child processes are invoked as
// `node <path>/node_modules/.../jest-worker/processChild.js`.
// The former is (obviously) easy to recognize, but the latter could happen at runtime as well. Fortunately, the main
// process hits this file before any of the child processes do, so we're able to set an env variable which the child
// processes can then check. During runtime, the main process is invoked as
// `node next start`
// or
// `node /var/runtime/index.js`,
// so we never drop into the `if` in the first place.
process.argv.includes('build') ||
process.env.SENTRY_BUILD_PHASE ||
// This is set by next, but not until partway through the build process, which is why we need the above checks. That
// said, in case this function isn't called until we're in a child process, it can serve as a good backup.
process.env.NEXT_PHASE === 'phase-production-build'
) {
process.env.SENTRY_BUILD_PHASE = 'true';
return true;
}
Expand Down