ref(nextjs): Make isBuild rely on NEXT_PHASE environment variable

[lforst]

Potentially (?) makes the isBuild utility method for our Next.js SDK more reliable, by looking at the NEXT_PHASE environment variable which is set by Next.js.

[lobsterkatie]

This works... almost. The reason I didn't use NEXT_PHASE to begin with is that it's not defined until partway through the build process:

That said, it seems like it's defined in all but the first isBuild invocation, so if we're sure that will always be the case, we could probably use NEXT_PHASE instead of our SENTRY_BUILD_PHASE. Then again, we know that what we have works, and will always work unless they change their CLI build command to be something other than next build (which feels relatively unlikely). Do you see an advantage in making the switch?

[lforst]

Ah thanks for showing me this. In my tests I only put console logs for isBuild inside all of the data fetcher wrappers... 🤦‍♂️

Do you see an advantage in making the switch?

It's probably fine the way it is right now. :D

[lforst]

Some more thoughts: It appears that the first invocation of isBuild happens in index.server.ts right at the start of the build. Why is it executing that file this early? We don't need to initialize the SDK during build. Makes me suspicious of whether we are injecting the sentry.client/server.config.js files in a little too many entrypoints. Maybe there are some files being generated that are only used for build and we inject the SDK into those too? Probably worth an investigation at some point.

[lobsterkatie]

It appears that the first invocation of isBuild happens in index.server.ts right at the start of the build. Why is it executing that file this early? Makes me suspicious of whether we are injecting the sentry.client/server.config.js files in a little too many entrypoints. Maybe there are some files being generated that are only used for build and we inject the SDK into those too?

No - we only inject into page files. Stick a debugger statement in right after we modify the entry property and you'll be able to see exactly what's injected where.

The reason it's running early isn't about initializing the SDK, it's about config. Here's the sequence of events:

• Next reads next.config.js
• In next.config.js, the user has imported withSentryConfig from the SDK
• Next loads the SDK index file
• At the top level of the file, we run isBuild() in our instrumentServer check

So, OTOH, if we end up deep-sixing instrumentServer, isBuild won't run that early and NEXT_PHASE being undefined won't matter. OTOH, it's totally possible that we might at some point have some other use for isBuild, and who knows if that'd run before or after NEXT_PHASE is defined. So I'd like to not rely solely on that. That said, if we wait to run isBuild in its current form, it might only run in child processes, at which point SENTRY_BUILD_PHASE will never get set. So I propose we go belt-and-suspenders style and use both: #5857.

We don't need to initialize the SDK during build.

This is probably true but a separate question, IMHO. But if we feel like it, we certainly could make Sentry.init() a no-op during build. The only reason I can think of not to do that is that it puts anyone who wants to instrument their CI (like we do) kind of out of luck. I don't have a strong opinion here, though.