feat(args): convert embedded and braced variables in command args #86
Conversation
Codecov Report
@@ Coverage Diff @@
## next #86 +/- ##
===================================
Coverage 100% 100%
===================================
Files 2 2
Lines 34 31 -3
===================================
- Hits 34 31 -3
Continue to review full report at Codecov.
|
| command = isWin ? `%${match[1]}%` : `$${match[1]}` | ||
| } | ||
| return command | ||
| return command.replace(envExtract, isWin ? '%$1$2%' : '$$$1') |
src/command.js
Outdated
| const envUseUnixRegex = /\$(\w+)/ // $my_var | ||
| const envUseWinRegex = /%(.*?)%/ // %my_var% | ||
| const envUseUnixRegex = /\$(\w+)|\${(\w+)}/g // $my_var or ${my_var} | ||
| const envUseWinRegex = /%(.*?)%/g // %my_var% |
There was a problem hiding this comment.
Do we need to be concerned about the g here based on your last PR? Perhaps we could just inline these regex declarations within the commandConvert function. I'm sure there's no relevant perf issues with that, and we could avoid the stateful nature of g.
There was a problem hiding this comment.
The issue from my last PR arises from the use of g and RegExp.prototype.exec. So here it is OK to use g because exec is not used anymore, and the test I added ensures the problem does not return in the future. That said, I also think inlining would be even safer, so I'll do that. Would you prefer that I keep them as variable inside commandConvert or that I inline them directly in the ternary on line 15?
There was a problem hiding this comment.
Keep them as a variable. That'll help it be self-documenting 👏
src/command.test.js
Outdated
| ) | ||
| }) | ||
|
|
||
| test(` |
There was a problem hiding this comment.
I'm guessing you had to do this because of linting? How about instead you just make it one line and add:
// eslint-disable-next-line max-len
On the line above this one. Would that be ok?
|
|
||
| test(`converts braced unix-style env variable usage for windows`, () => { | ||
| isWindowsMock.__mock.returnValue = true | ||
| // eslint-disable-next-line no-template-curly-in-string |
There was a problem hiding this comment.
Haha, one of the few cases where this rule needs to be disable 😆
|
I changed the base of this PR from |
|
Actually, if you wanna do this in another PR to |
hgwood
force-pushed
the
feat/multi-env-vars
branch
from
c42b9e6 to
68bf464
Compare
|
Thanks for the review. I will have a look at those other PR/issues either later tonight or tomorrow, and see what I can do. |
|
Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the CONTRIBUTING.md file (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :) |
* feat(args): convert variables embedded in command args * feat(args): convert braced variables in command args * style(args): full test name on one line * refactor(args): make commandConvert definitly stateless BREAKING CHANGE: `echo $var2/$var1` would not be changed on windows, now it is. This is kind of a bug, but we're doing a major version bump to be safe.
Hello again. :)
Now for the actual contribution I mentioned in #85. It turns out it supersedes #85, but I submitted #85 anyway because I thought it could still be useful for you to make a patch release, and you did, so that's great! This contribution however, is a breaking change.
If you feed cross-env on Windows with something like
echo $npm_package_name $npm_package_version, it correctly runsecho %npm_package_name% %npm_package_version%. But if instead you feed it withecho $npm_package_name/$npm_package_version(notice the slash in the middle), then it runsecho %npm_package_name%. That is because the command argument is replaced by the converted first match of a variable pattern. The goal of this PR is to make cross-env support variables embedded inside arguments, so that it would instead runecho %npm_package_name%/%npm_package_version%(breaking change).Supporting embedded variables means cross-env also needs to support braced variables on Unix (
${myvar}), so this PR introduces that as well.Let me know if you find this feature useful.