-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Isaacs/set envs fix #1652
Isaacs/set envs fix #1652
Conversation
c682b87
to
8209b29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubber Stamp LGTM
Tested locally and it fixes the bug I was running into re: missing env var
} | ||
|
||
const envVal = val => Array.isArray(val) ? val.join('\n\n') : val | ||
const envVal = val => Array.isArray(val) ? val.map(v => envVal(v)).join('\n\n') | ||
: val === null || val === undefined || val === false ? '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
o_O can we at least start nesting these? ex.
const envVal = val => Array.isArray(val) ? val.map(v => envVal(v)).join('\n\n')
: val === null || val === undefined || val === false ? ''
: typeof val === 'object' ? null
: String(val)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've moved to lining up the :
for easier scanning down a straight left-edge, so the CLI matches all the other repos.
Fix #1650 PR-URL: #1652 Credit: @isaacs Close: #1652 Reviewed-by: @MylesBorins
Landed and published on 7.0.0-beta.4. |
Implement the changes in npm/rfcs#183, and set environment variables properly when they are not strings.
Also adds tests for the set-envs util.