fix: Resolve value of env variables before invoking cross-spawn #95
Conversation
src/index.js
Outdated
| return [command, commandArgs, envVars] | ||
| const commandArgs = args.slice() | ||
| const command = commandConvert(getCommand(commandArgs, envVars)) | ||
| return [commandConvert(command), commandArgs.map(commandConvert), envVars] |
There was a problem hiding this comment.
mapdoes not mutate its argument so no need toslicefirst- So the command is converted twice? Is that on purpose?
There was a problem hiding this comment.
So the command is converted twice? Is that on purpose?
No, that was a mistake. Fixed. Good catch!
map does not mutate its argument so no need to slice first
getCommand mutates its first argument (commandArgs), so the .slice is necessary.
There was a problem hiding this comment.
Oh, I see. You are leveraging the fact that getCommand will strip env setters and the command from commandArgs. And you are making a copy so that getCommandArgsAndEnvVars does not itself mutate its argument. Am I correct? I think this is not very clear from the code. I understand it is done like this to minimize impact on existing code. It might still be a good idea to rename getCommand to something more revealing. However, the more I think about a name for this, the more I realize it does too much to have a good name:
- it strips env setters and the command from its first argument
- it sets env vars on its second argument
- it returns the command
A possible improvement would be to replace it with 2 functions:
- args -> [env setters, command, commandArgs] (simple to use using destructuring)
- env setters -> map of env vars (to be merged with
process.envto form the env for the new process)
What do you think? Is it worth the work?
There was a problem hiding this comment.
You are leveraging the fact that getCommand will strip env setters and the command from commandArgs. And you are making a copy so that getCommandArgsAndEnvVars does not itself mutate its argument. Am I correct?
Correct. If I didn't make a copy, a call to crossEnv would get its arguments mutated, and I don't think we want that :)
A possible improvement would be to replace it with 2 functions...
That sound good, this part was the most confusing when I started looking at this repo, so I think it's totally worth it to refactor a bit.
There was a problem hiding this comment.
In addition, we could probably refactor out the whole loop to simplify the code a bit and remove the mutation. 😀
|
What do you folks think about it now? 53776d3 I believe it's a bit cleaner now but I've written it 5 minutes ago so I may be a bit biased :) |
|
Hey @danielo515, 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 :) |
|
Dear @kentcdodds Thank you very much. I'm not sure why are no notifying me on this pull request (convenience I guess), but I'm glad to become a collaborator. Regards |
|
@danielo515 I think he mispelled my name haha |
|
😅 whoops! Yeah, sorry about that @danielo515, I appreciate your contributions as well! I have a fairly open contributors policy so if anything comes up, feel free to jump in with a PR. I generally give contrib rights to anyone who's contributed non-trivial code changes :) |
* fix: Resolve value of env variables before invoking cross-spawn #90 * Refactored the main parsing loop BREAKING CHANGE: This is unlikely to break anyone, but now if you assign a variable to a variable (like `FOO=$BAR` with the value `$BAR` being assigned to `hello`, the command will be converted to `FOO=hello` whereas before it was `FOO=$BAR`).
Since this was the only blocker for releasing the next major version, I decided to give it a try.
Before:
cross-env FOO=$NODE_ENV echo $FOOOutput (before this PR):
"$NODE_ENV"on UNIX,"%NODE_ENV%"on Windows.Output (after this PR):
"development"(or whatever value NODE_ENV has)I've added some logic in the part of the parser that processes the right-hand side of variable assignments (in this example, the
$NODE_ENVin theFOO=$NODE_ENVexpression). It will find all occurences of UNIX-style env variables and replace them with their value.I've also had to change the program logic a little bit so
commandConvertonly operates on the real command and its arguments, not on the variable assignments.Fixes #90