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

Conversation

lobsterkatie
Copy link
Member

This makes a few changes to the isBuild utility function in the nextjs SDK in order to make its operation less brittle. Currently, it relies on being called at the top level of index.server.ts, which ensures that it runs early enough in the build process to be executed in the main thread, before operation has been split out into child processes. (This is important because child processes don't conveniently have the word 'build' in their invocations the way the main thread (invoked by next build) does, removing one of isBuild's clues about what phase it's in.) It then sets an environment variable as a clue to future calls to itself, so that, child processes or not, it knows the correct phase. This means, however, that if it stops being used at index.server.ts's top level, and doesn't get called until after the process split, it won't have any way to know whether it's in the build phase or not.

This makes two changes to guard against those possibilities:

  • To ensure that it's always called as early as possible in the build, it is now run independently of any use of its return value, instead storing that value in a constant, which is then used anywhere it's needed.
  • To provide a backup indicator of the current phase, it now also checks the next-provided NEXT_PHASE environment variable. (The reason it doesn't simply rely on this variable to begin with is that it's only set partway through the build process.)

As a result of these changes, two other changes are included here:

  • The export of isBuild is deprecated in favor of the computed constant. (Since they're equivalent, no reason to keep exporting them both.)
  • The local constant isVercel has been renamed IS_VERCEL, to match IS_BUILD. (Yes, I'm that kind of OCD.)

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Capitalization 😎

*/
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.

@lobsterkatie lobsterkatie merged commit 2b8237c into master Oct 4, 2022
@lobsterkatie lobsterkatie deleted the kmclb-nextjs-make-isBuild-more-robust branch October 4, 2022 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants