feat: add default value handling and PWD resolution #219
Conversation
Codecov Report
@@ Coverage Diff @@
## master #219 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 4 5 +1
Lines 78 132 +54
Branches 18 35 +17
=====================================
+ Hits 78 132 +54
Continue to review full report at Codecov.
|
yinzara
force-pushed
the
pr/default-values
branch
from
c09761e
to
627e676
Compare
yinzara
changed the title
|
Thanks for this! I'm concerned that the shell may eagerly replace these values before they get to |
|
@kentcdodds, does that practically matter? If the shell converts them, all good, if you're not on a shell that understands that syntax, |
|
I had that thought as well. But I just want to be sure. |
|
Yeah I agree. I'll make an example in both Windows PowerShell and Bash in Linux and we'll see if it comes out with the same results but I'm pretty sure it will. However, as stated, since I'm just duplicating the functionality of what Bash already supports, I can't see how it wouldn't function identically. |
|
I guess the only issue would be if the timing matters for some reason. Like if some insane person does |
|
Good news and bad news. Good news. CMD does not expand the variables. If you run cross-env directly from PowerShell, it DOES eagerly expands them but it does this already (i.e. cross-env already doesn't work correctly with PowerShell directly) because variables in PowerShell start with a $. However, if you use However, there is bad news. When I started to use more complex examples of replacements it turns out the regex style approach I was using has examples that break. I will add test cases to cover this and see if I can rework the approach. |
|
Not bad. I need to add some test cases to keep us at 100% (I won't say this is good until then). |
|
CMD.exe |
|
Windows PowerShell |
|
Mac OS X Bash |
|
Any chance I could get some love on this? I really want to use this with my team but keeping the custom version is a maintenance nightmare. |
| **/ | ||
| // state machine requires slightly more complexity than normally required | ||
| // eslint-disable-next-line complexity | ||
| function envReplace(value, env = process.env, winEnvReplace = false) { |
There was a problem hiding this comment.
This is amazing, but I really don't want to maintain this function. And environment variable expansion syntax is not something I'm very familiar with.
Would you agree to help maintain this code if I merge?
There was a problem hiding this comment.
An alternative might be to use a PEG parser generator or something. Let me see if I can come up with something, just to see what it looks like. Total props for writing this though @yinzara, not having a dig at all. I've written my fare share of this kind of thing, and they are hard!
There was a problem hiding this comment.
Honestly I'm happy to help maintain what I've written. Love to see your PEG variation if you would like. Up you you
There was a problem hiding this comment.
Sure, got something mostly working now, but need to support escape characters and stuff.
There was a problem hiding this comment.
This is what I've got in mind. I'm not totally done yet and I've got to head out now for a bit, but this is pretty close. Just need to sort out a few final cases like ${BOB:-hello " there"} and make sure it behaves like bash. You can load this up and play with it here: https://pegjs.org/online.
{
const process = {
env: {
BOB: "Hello!"
}
};
function getEnv(name, fallback) {
const env = process.env[name];
return (env == null || env === '')
? (fallback == null ? '' : fallback)
: env;
}
}
start
= String
String = QuotedString / UnquotedString
EnvVar
= "${" _ name:VarName _ (":-" / ":=") fallback:FallbackValue "}" { return getEnv(name, fallback); }
/ "${" _ name:VarName _ "}" { return getEnv(name); }
/ "$" name:VarName { return getEnv(name); }
/ "$" { return "$"; }
FallbackValue
= QuotedString
/ val:((BracketStringChar / EnvVar)*) { return val.join('').trim(); }
VarName "environment variable name"
= [a-zA-Z_][a-zA-z_0-9]+ { return text(); }
_ "whitespace"
= [ \t\n\r\v]*
QuotedString
= '"' val:(DoubleStringChar / EnvVar)* '"' { return val.join(''); }
/ "'" val:(SingleStringChar / EnvVar)* "'" { return val.join(''); }
UnquotedString
= val:(BareStringChar / EnvVar)* { return val.join(''); }
DoubleStringChar
= '\\' escaped:("$" / '"') { return escaped; }
/ !("$" / '"') char:. { return char; }
SingleStringChar
= "\\" escaped:("$" / "'") { return escaped; }
/ !("$" / "'") char:. { return char; }
BracketStringChar
= "\\" escaped:("$" / "}") { return escaped; }
/ !("$" / "}") char:. { return char; }
BareStringChar
= "\\" escaped:("$") { return escaped; }
/ !("$") char:. { return char; }
There was a problem hiding this comment.
That's great to hear, thanks! I've been wanting an excuse to build a parser for a while, so I'm happy to have found a good reason.
I agree that setting the value as part of the stringify function would be wrong, but my thought was to create a few different methods for walking the parse tree, one of which would be a more "interactive" style thing to set environment along the way. That's the next thing I'm gonna look at.
The brace matching should all be handled automatically by the grammar. There's something like this (I'm simplifying a bit) under the Variable production rule: "${" VarName ":-" Fallback "}", and the Fallback can be 0 or more Word / Quoted / Variable, so they're mutually recursive and can nest indefinitely. If for some reason it fails to match then it falls back on just reading it as plain text, although bash refuses to parse, so I'm still on the fence about which way to go there.
There was a problem hiding this comment.
In terms of helping, I'd love that, but maybe in a little bit once I get things settled 😜. I'm still working pretty rough and ready at the moment, but hope to get it into a relatively stable state soon.
There was a problem hiding this comment.
Love it. I think this will drastically simplify cross-env (and any other library that attempts to do something with args like this).
There was a problem hiding this comment.
That's great, thanks a lot for the feedback! I sometimes find it difficult to tell whether my vision for something is as good as I think it is, or I'm just getting lost in the woods, so the validation is great. This feels pretty nice and clean though, so I'm happy to hear that.
I'll keep plugging away at it, and probably try and integrate it with a cross-env branch next week to see how that goes.
There's got to be somewhere better to talk about it than this PR though, but Github doesn't really have room for random chats. I might make a "cross-env integration" issue or something in the package that we can all continue talking on. Would that be better?
There was a problem hiding this comment.
We can continue here: franklin-ross/bash-env-parser#7
|
Any updates on this? |
|
Hey, my apologies. I got a bit lost in my new workplace. I'm going to jump back on this and create a PR. |
|
I have to admit that I initially balked when I realised exactly how messy this all is. I believe the library I've written is quite true to bash, but the nature of I need to get over that and accept that I'm afraid that this will increase the (already significant) number of issues raised to the tune of "this doesn't work for this corner case". I think it's still worth it, but I want to point that out. |
|
Would be great if we could get this feature merged. |
|
Thanks @kentcdodds for all your work. Totally understand about this. |
What:
Adds support for specifying default values for environment variables using the standard POSIX/bash syntax
${ENV_VAR_NAME:-default value}. Also support recursive resolution through defaults (i.e.${ENV_VAR_NAME:-${OTHER_ENV_VAR:-default if both not set}})Also adds support for resolution of ${PWD}/$PWD into the current working directory.
Why:
Currently there is no way with the library to specify a default value for an environment variable when not present. There is a reference in the readme to another library that seems to be a fork of this library with the added feature however I can't find the source for it and the documentation for the library incorrectly references this library. Additionally how they suggest you specify a default value (i.e.
${ENV_VAR_NAME:default value}) doesn't follow standard POSIX/bash so there is no portability of this syntax beyondcross-env.In most linux/unix shells the PWD environment variable points to the current working directory but in Windows no such environment variable exists. This PR also adds resolution in windows using the NodeJS
process.cwd()function.How:
This required a rewrite of the primary regular expression based implementation of value replacement to a combination iterative/recursive algorithm.
To implement the recursive replacement in the same fashion as Bash does (so that if the value was evaluated in Bash before it wouldn't cause functional changes), I had to use a brace matching iterative state machine over the characters of the string that recursed on the default values calculation.
This allowed me to replace the double implementation (in command.js and variable.js) of the string replacement algorithm. Previously there were two nearly identical implementations that only varied in Windows and for the command.js only.
I've created a new "env-replace.js" that exports a function that achieves this for both. It's documented in the file. All existing test cases pass with no changes and a new test case has been added for an extremely complex case:
start-${PWD}-${EMPTY_VAR:-${NO_VAR:-$VAR1}}-${NO_VAR:-$VAR2}-${NO_VAR:-${EMPTY_VAR:-value3}}-${EMPTY_VAR:-$PWD}-endwhich now passes. I will provide screen shots of this evaluating in both PowerShell (through a npm script) and Mac OS Bash (both in the console in via an npm script).Checklist: