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

NPM script interoperability : allow simple quotes in command arguments for Windows platform #98

Closed
wants to merge 1 commit into from

Conversation

Drozerah
Copy link

@Drozerah Drozerah commented Oct 24, 2019

  • index.js:
    • check if process.platform is equal to 'win32' then:
    • remove generated simple quotes for each elements in matches
      array then:
    • update original matches array
  • README.md:
    • remove text paragraph about Windows users and double quotes

- index.js:
	- check if process.platform is equal to 'win32' then:
	- remove generated simple quote for each elements in matches
	array then:
	- update original matches array
- README.md:
	- remove text paragraph about Windows users and double quotes
Copy link
Collaborator

@blakeembrey blakeembrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not exactly clear if or what this fixes to be honest. Why shouldn’t you use double quotes?

Code nit: You could use map.

@Drozerah
Copy link
Author

@blakeembrey

Why shouldn’t you use double quotes?

Please read carefully the PR title => "allow simple quotes in command arguments for Windows platform"

Means that if you don't use double quotes on Windows, onchange will simply ignore the command line arguments without any error message or warning, means that you'll have no way to understand why the files you want to listen won't be listened. On the other hand, this is a limitation for NPM script interoperability and code sharing... Last, the PR doesn't bring breaking change, double quotes continue to work in NPM script as well as simple quotes...

@blakeembrey
Copy link
Collaborator

But why shouldn't you just use double quotes instead? We can change the code samples to use double quotes in case you're worried it's from copying the examples. I have concerns about a blanket replace like this for two reasons:

  1. You're not trimming, you're replacing anywhere in the string (which might remove characters someone cares about)
  2. This would not change quotes anywhere else in the command, resulting in confusingly inconsistent behavior (e.g. the command will work differently than the match parameters)
  3. Different shells on Windows operate differently. I haven't gone and tested this, but will before I intend to merge this, but I do know Powershell supports both style of quotes.

Which shell are you using on Windows that this is a problem? I'd like to replicate it and add a test first.

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

Successfully merging this pull request may close these issues.

2 participants