Change operator || to ?? where reading environmental valuable [Fixes #12057]
#12058
Conversation
HiroyukiNaito
requested review from
wackerow,
corwintines,
pettinarip,
minimalsm and
nhsz
as code owners
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Hey, thanks for the PR @HiroyukiNaito
I agree with the const LIMIT_CPUS = Number(process.env.LIMIT_CPUS ?? 2) change, since notably the nullish coalescing operator ?? will allow 0 to be passed without it being treated as falsy (which we want).
Oddly, I have my LIMIT_CPUS env var set to 0 locally and this is still working (using 0 instead of 2)... not totally sure why, but I'm in favor of updating this to the more appropriate operand.
The other two I don't think we need/want.
?? false is redundant... In either case, this value will either be undefined (if env var not declared) or an empty string "" (if env var declared but left blank), both of which will be treated as "falsy" in the ternary expression beneath it. This is the desired outcome, as both of these should fall back to using all locales.
In the case of the siteUrl, we would NOT want to accept an empty string here...
Similar to how 0 is accepted with ??, an empty string is also accepted, and will not fall back to the right-hand of the expression. Using || will treat "" or 0 as falsy and move to the right side or the operand to use the hard-coded fallback.
Let me know if that makes sense, or if I'm missing something. cc @pettinarip curious if you have thoughts
|
@wackerow Thank you for the deep insight. |
|
@wackerow |
Change operator
||to??where reading environmental valuable [Fixes #12057]Description
When reading environmental valuables, it is safer to use '??' operator.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing
Related Issue
#12057