-
Notifications
You must be signed in to change notification settings - Fork 243
Possible regression with 4.0.0, && operator have a strange behaviour #99
Comments
Thanks, I've managed to reproduce it with your instructions. It looks like a bug indeed :). After debugging a little, I've found the cause: This command: I don't have time to solve this right now (contributions welcome!), but I've found a workaround that may get you by: |
Thanks a lot for your response and the workaround, I will look if I can make a PR for the fix 😃 |
Shouldn't the |
I'm not sure if it's a bug or what we actually want. 🤔 |
We were warned. But I misunderstood the warning. I thought it meant "prefer cross-spawn's |
I think it's definitely a bug. Taking the script on the example: Actually the arguments passed to
Which, if they were properly escaped by If the problem is with |
If we can verify that doing that wont break a bunch of use cases, then I'm all for it. |
I just hit a possible duplicate of this bug after upgrading from 3.2.4 to 4.0.0. I have the following npm script:
With 3.2.4 this works fine, but with 4.0.0 I get an error from rollup saying |
@DanReyLop Thanks for the workaround, with cross-env 4.0.0, I am able to workaround the issue by changing my npm script to the following:
|
@DanReyLop I agree it is a bug. cross-env users expect everything after env setters to be passed to their shell verbatim (except for variable syntax conversion of course). You are right that cross-spawn is bringing nothing more than compatibility with Node <4.8. It does nothing more, nothing less. Which is to say that the problem is not with cross-spawn. We could drop it and use When the This is what we could do:
Obviously, that would be a breaking change. Fixing this bug can only be a breaking change. I think we also need stronger tests to avoid this kind of problems. The use of cross-spawn prevented us from really testing the command we produced. Without it we could have better end-to-end tests (given this @DanReyLop @kentcdodds @ebriand Thoughts? I should have time to work on this in the coming week. |
My bad, I misead how
It wouldn't be as simple as that, because
And then escaped using |
If we do that we'll definer want thorough testing to make sure the variable replacement doesn't really in an invalid command. But I'm good with it if we can achieve that. Would rather do something else though... |
So maybe it's not shell-quotes that we need but something else. It should put quotes around arguments and escapes existing quotes inside the args. Eg:
Should it do something else that I'm missing? |
So it does not rely on quotation logic to pass.
Just to clarify how the workaround works:
|
FYI, discussion on solutions continued over at #102. |
Revert the default bin (cross-env) to its v3 behavior (not using the shell option). Add a new (cross-env-shell) which uses the shell option. BREAKING CHANGE: Scripts using quotes or escape sequences will see a difference in behavior. Switching to the second bin should resolve any issue. Closes kentcdodds#99.
I ran into this or a related issue by using this command line: cross-env DEBUG=myapp:* nodemon It works on 3.x, but it seems like it breaks on the colon ( console.log(process.env.DEBUG);
myapp;* Notice the Indeed, when I try |
This was a documented breaking change to support windows better:
So I think this should work:
|
I guess I should've RTFM. Thanks for the clarification. |
Revert the default bin (cross-env) to its v3 behavior (not using the shell option). Add a new (cross-env-shell) which uses the shell option. BREAKING CHANGE: Scripts using quotes or escape sequences will see a difference in behavior. Switching to the second bin should resolve any issue. Closes kentcdodds#99.
Revert the default bin (cross-env) to its v3 behavior (not using the shell option). Add a new (cross-env-shell) which uses the shell option. BREAKING CHANGE: Scripts using quotes or escape sequences will see a difference in behavior. Switching to the second bin should resolve any issue. Closes #99.
Hi
I'm running on windows 7 with node 7.7.3. I updated cross-env to 4.0.0 and migrate my npm scripts but one of my script doesn't work anymore. I tested this behavior with an empty project :
With cross-env 3.2.4 and the npm script :
output :
With cross-env 4.0.0 and the script :
output :
Maybe I'm missing something ?
The text was updated successfully, but these errors were encountered: