-
Notifications
You must be signed in to change notification settings - Fork 3.5k
feat: add support for optional env var replacements in .npmrc #8359
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
base: latest
Are you sure you want to change the base?
feat: add support for optional env var replacements in .npmrc #8359
Conversation
I know linting checks for this but I also manually validated via https://devina.io/redos-checker |
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.
Looks like a great feature! Just a couple nits
Environment variables can be replaced using `${VARIABLE_NAME}`. For | ||
Environment variables can be replaced using `${VARIABLE_NAME}`. By default | ||
if the variable is not defined, it is left unreplaced. By adding `?` after | ||
variable name they can be forced to evaluate to an empty string instead.For |
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.
variable name they can be forced to evaluate to an empty string instead.For | |
variable name they can be forced to evaluate to an empty string instead. For |
@@ -1,9 +1,11 @@ | |||
// replace any ${ENV} values with the appropriate environ. | |||
// optional "?" modifier can be used like this: ${ENV?} so in case of the variable being not defined, it evaluates into empty string |
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.
// optional "?" modifier can be used like this: ${ENV?} so in case of the variable being not defined, it evaluates into empty string | |
// optional "?" modifier can be used like this: ${ENV?} so in case of the variable being not defined, it evaluates into empty string. |
t.equal(envReplace('\\${foo}', env), '${foo}') | ||
t.equal(envReplace('\\\\${foo}', env), '\\bar') | ||
t.equal(envReplace('\\\\\\${foo}', env), '\\${foo}') | ||
t.equal(envReplace('${baz}', env), '${baz}') | ||
t.equal(envReplace('\\${baz}', env), '${baz}') | ||
t.equal(envReplace('\\\\${baz}', env), '\\${baz}') | ||
t.equal(envReplace('\\${foo?}', env), '${foo?}') | ||
t.equal(envReplace('\\\\${foo?}', env), '\\bar') | ||
t.equal(envReplace('${baz?}', env), '') | ||
t.equal(envReplace('\\${baz?}', env), '${baz?}') | ||
t.equal(envReplace('\\\\${baz?}', env), '\\') |
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.
Suggestion for a couple more tests here - and what do you think about adding a description, I find it way easier to read:
t.equal(envReplace('\\${foo}', env), '${foo}') | |
t.equal(envReplace('\\\\${foo}', env), '\\bar') | |
t.equal(envReplace('\\\\\\${foo}', env), '\\${foo}') | |
t.equal(envReplace('${baz}', env), '${baz}') | |
t.equal(envReplace('\\${baz}', env), '${baz}') | |
t.equal(envReplace('\\\\${baz}', env), '\\${baz}') | |
t.equal(envReplace('\\${foo?}', env), '${foo?}') | |
t.equal(envReplace('\\\\${foo?}', env), '\\bar') | |
t.equal(envReplace('${baz?}', env), '') | |
t.equal(envReplace('\\${baz?}', env), '${baz?}') | |
t.equal(envReplace('\\\\${baz?}', env), '\\') | |
t.equal(envReplace('${foo}', env), 'bar', 'replaces defined variable') | |
t.equal(envReplace('${foo?}', env), 'bar', 'replaces defined variable with ? modifier') | |
t.equal(envReplace('${foo}${bar}', env), 'barbaz', 'replaces multiple defined variables') | |
t.equal(envReplace('${foo?}${baz?}', env), 'bar', 'replaces mixed defined/undefined variables with ? modifier') | |
t.equal(envReplace('\\${foo}', env), '${foo}', 'escapes normal variable') | |
t.equal(envReplace('\\\\${foo}', env), '\\bar', 'double escape allows replacement') | |
t.equal(envReplace('\\\\\\${foo}', env), '\\${foo}', 'triple escape prevents replacement') | |
t.equal(envReplace('${baz}', env), '${baz}', 'leaves undefined variable unreplaced') | |
t.equal(envReplace('\\${baz}', env), '${baz}', 'escapes undefined variable') | |
t.equal(envReplace('\\\\${baz}', env), '\\${baz}', 'double escape with undefined variable') | |
t.equal(envReplace('\\${foo?}', env), '${foo?}', 'escapes optional variable') | |
t.equal(envReplace('\\\\${foo?}', env), '\\bar', 'double escape allows optional replacement') | |
t.equal(envReplace('${baz?}', env), '', 'replaces undefined variable with empty string when using ? modifier') | |
t.equal(envReplace('\\${baz?}', env), '${baz?}', 'escapes undefined optional variable') | |
t.equal(envReplace('\\\\${baz?}', env), '\\', 'double escape with undefined optional variable results in empty replacement') | |
This solves problem described in #8335 in a backwards-compatible way.
This PR adds possibility to have env var replacements in .npmrc configs written as
${VAR?}
which will cause them to get replaced with an empty string if the variable is not defined. Old behavior where undefined variables are left unreplaced is not changed.References
Fixes #8335