Check if process exists before accessing it #1784
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
handeyeco
changed the title
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (84fd157) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1784If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1784 |
|
Size Change: +3.38 kB (+0.39%) Total Size: 870 kB
ℹ️ View Unchanged
|
| @@ -15,10 +15,14 @@ type I18nContextType = { | |||
| locale: string; | |||
| }; | |||
|
|
|||
| const shouldMockStrings: boolean = Boolean( | |||
| process?.env?.NODE_ENV === "test" || process?.env?.STORYBOOK, | |||
There was a problem hiding this comment.
I don't think this will do what you want. If the process global variable doesn't exist, accessing it directly will fail. process?.env just returns undefined if process is nullish, but in order for that to work the variable process has to exist.
I think this will work:
typeof process !== "undefined" && (process?.env?.NODE_ENV === "test" || ... )However, storing the result of a process.env check in a variable like this will prevent most build tools from doing dead code elimination. Vite, webpack, etc. will optimize an expression like:
process.env.NODE_ENV === "test" ? foo : bar
to foo in test environments and bar elsewhere. The env check won't actually appear in the built code. This behavior might be why this code currently works in browsers.
If the env check is abstracted at all, dead code elimination generally won't happen. I think that doesn't matter in this case, because the amount of code that would be eliminated here is tiny and not side-effecting, but I think as a general practice we'd want to keep env checks inline.
There was a problem hiding this comment.
Yeah, it didn't feel right (and was breaking the SB build). Thanks for the feedback, fixed.
This reverts commit a8f0165.
## Summary: Reverts #1784 because it broke (static) Storybook Author: handeyeco Reviewers: jeremywiebe Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, 🚫 Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Check for .changeset entries for all changed files (ubuntu-latest, 20.x), 🚫 Cypress (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ gerald, 🚫 Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), 🚫 Cypress (ubuntu-latest, 20.x), 🚫 Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, 🚫 Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Cypress (ubuntu-latest, 20.x), 🚫 Check for .changeset entries for all changed files (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1790
Summary:
processis a NodeJS thing, so it's not guaranteed to be on the FE. This checks that it exists before accessing it.