Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows backslashes unescape appears since version 1.6.3 without possibility to disable it #37

Closed
paztis opened this issue Aug 19, 2019 · 10 comments

Comments

@paztis
Copy link

@paztis paztis commented Aug 19, 2019

Since 1.6.3 (commit c5b7ca4), backslashes unescape has been added
s = s.replace(/\\\\/g, '\\')

There's no option to disable it, and it is a non backward compatible change in a patch version
Projects like npm-run-all are pointing to ^1.6.0 version of shell-quote and this totally broke its functionality for me.
I was not escaping the backslashes before, and not the backslashes totally disapears on my code...

You might only do this kind of changes on a major version version (2.0.0), or provide an option to disable it.

@binki
Copy link

@binki binki commented Aug 22, 2019

Instead of trying to escape shell arguments using a utility like this, you should probably be using an API like that provided by cross-spawn where you can pass your arguments as an array.

@paztis
Copy link
Author

@paztis paztis commented Aug 22, 2019

I don't use directly shell-quote. I use npm-run-all that permits you to chain scripts easily directly from package.json.
I use frequently cross-spawn but here I don't want to create a js file each time to chain 2 calls.

More npm-run-all also uses cross-spawn. Shell-quotes is only used to parsing the command line.

So for me, the problematic is without changing the version a npm-run-all, one nested dependency patch version change brokes the result. I can control the versions of my dependencies, but not for now the nested ones.

That's why I ask here to publish a rollback of this code under 1.7.x and to republish this code under 2.0.0

@goto-bus-stop
Copy link
Collaborator

@goto-bus-stop goto-bus-stop commented Aug 27, 2019

Sorry, I wasn't watching issues on this repo. I'll look into this today.

@paztis
Copy link
Author

@paztis paztis commented Aug 27, 2019

thanks

@paztis
Copy link
Author

@paztis paztis commented Aug 28, 2019

Have you found time to look at this issue ? How do you think you will resolve it ?

goto-bus-stop added a commit to goto-bus-stop/node-shell-quote that referenced this issue Aug 30, 2019
@goto-bus-stop
Copy link
Collaborator

@goto-bus-stop goto-bus-stop commented Aug 30, 2019

Just reverting that change for now, I'm not sure what the best behaviour there would be. The unescaping can lead to funky things in linux bash as well.

Thanks for reporting!
📦 1.7.2

@paztis
Copy link
Author

@paztis paztis commented Aug 30, 2019

@paztis
Copy link
Author

@paztis paztis commented Aug 31, 2019

Have you publish version 1.7.2 on npm ? Seems to be still in 1.7.1.

@goto-bus-stop
Copy link
Collaborator

@goto-bus-stop goto-bus-stop commented Sep 1, 2019

oh, I'm on a new laptop and didn't configure npm yet, so the npm publish errored out and i didn't notice! thanks for the ping, i'll do it right now.

@paztis
Copy link
Author

@paztis paztis commented Sep 1, 2019

Perfect. Seems released now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants