Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

Resolving vars in other vars #90

Closed
hgwood opened this issue Mar 15, 2017 · 5 comments · Fixed by #87
Closed

Resolving vars in other vars #90

hgwood opened this issue Mar 15, 2017 · 5 comments · Fixed by #87

Comments

@hgwood
Copy link
Collaborator

hgwood commented Mar 15, 2017

On master, cross-env MY_VAR=$MY_OTHER_VAR echo $MY_VAR on Windows results in this command: %MY_OTHER_VAR% echo %MY_VAR%, which is incorrect. #86 fixes this.

On next (i.e. with #86 merged), it results in echo %MY_VAR%, which is correct. However, in the spawned process, process.env.MY_VAR is %MY_OTHER_VAR%, which is not very useful, and I would expect it to be the value of MY_OTHER_VAR from the environment in which cross-env is run.

An easy solution would be to replace variables by their value instead of their platform-specific form (cross-var does this), but I'm not sure that's what we want. In this scenario, cross-env MY_VAR=$MY_OTHER_VAR echo $MY_VAR becomes echo <value of MY_OTHER_VAR>.

Another solution is replace variables by their value only in the part of the command that sets new variables. In this scenario, cross-env MY_VAR=$MY_OTHER_VAR echo $MY_VAR becomes echo %MY_VAR% like today but MY_VAR is properly set to <value of MY_OTHER_VAR>. I think this is better, but it requires more work as commandConvert needs to be split into two or cover the two cases.

What do you think?

@kentcdodds
Copy link
Owner

Yes, let's Make this happen for the next major release 👍

@hgwood
Copy link
Collaborator Author

hgwood commented Mar 15, 2017

I realized:

  • cross-env seems to support converting Windows vars to Unix vars, even though this is not mentioned in the readme. Perhaps this feature could be dropped to reduce complexity. I think it is probably not used (still a breaking change).
  • Now that cross-env uses the shell option of spawn, if cross-env is run on Unix, then the command string passed to cross-env can be passed to spawn verbatim. Then it becomes even more interesting to drop Windows->Unix conversion because we wouldn't even need to check anything if the platform is Unix.

What do you think?

@kentcdodds
Copy link
Owner

kentcdodds commented Mar 15, 2017

converting Windows vars to Unix vars,

Yeah, I noticed that (sometimes you forget when so many people have contributed). Let's drop it. It was never intended to go both ways and it's confusing that it does.

[no need to] check anything if the platform is Unix.

Awesome, let's make that happen too! Thanks for all your help on this! 👍

@kentcdodds
Copy link
Owner

Thanks to #94 by @DanReyLop, the issues you brought up are no longer a problem. Now implementing this "Resolving vars in other vars" thing should be more straightforward. This is the last thing we're waiting on for the release. Do you want to work on it?

@hgwood
Copy link
Collaborator Author

hgwood commented Mar 26, 2017

I planned to but looks like @DanReyLop needs this release out faster than me ;)

DanReyLop pushed a commit to Automattic/cross-env that referenced this issue Mar 26, 2017
kentcdodds pushed a commit that referenced this issue Mar 27, 2017
* 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`).
kentcdodds pushed a commit that referenced this issue Mar 31, 2017
* 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`).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants