opened
johanneswuerbach
username_0
console.log\(\"hello\"\)
        ^^^^
SyntaxError: Unexpected token ILLEGAL
    at Object.exports.runInThisContext (vm.js:53:16)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:409:26)
    at node.js:579:27
    at nextTickCallbackWith0Args (node.js:420:9)
    at process._tickCallback (node.js:349:13)
```
Different escaping between spawn and cross-spawn
issue
created
satazor
username_1
That's strange indeed. At the moment, I'm overwhelmed with work. Can you pinpoint where the issue is?
Thanks
comment
created
johanneswuerbach
username_0
@username_1 send a test + fix here: https://github.com/IndigoUnited/node-cross-spawn/issues/43
comment
created
satazor
username_1
@username_0 Running your example on Win10 throws me an error:

```
spawn [eval]:1
console.log\("hello"\)
```

The correct way is:

```js
const cp1 = spawn('node', ['-e', 'console.log(\\"hello\\")'], { shell: true });

cp1.stdout.on('data', (data) => console.log('spawn', data.toString()));
cp1.stderr.on('data', (data) => console.log('spawn', data.toString()));
```

Anyway, you are right that the behavior between spawn and cross-spawn is not consistent when `options.shell` is provided. I've taken a look at your PR but I think we should do NO escaping/manipulation whatsoever when `options.shell` is specified so that we mach NodeJS [behavior](https://github.com/nodejs/node/blob/master/lib/child_process.js#L326-L343).

I'm currently working on a PR based on your code.
comment
created
johanneswuerbach
username_0
...
/bin/sh: -c: line 0: syntax error near unexpected token `('
/bin/sh: -c: line 0: `node -e console.log(\"hello\")'
```

as the `(` also needs to be escaped.

While I could modify the command for both platforms, I thought cross-spawn should handle this somehow automagically.
comment
created
satazor
username_1
Thinking out of the box, I think the correct call here is not to support the `shell` option.

Let me elaborate.. `cross-spawn` uses a shell on windows (cmd.exe) and does escape the command and args as necessary:

- If user passes `options.shell=false`, what's the behavior that cross-spawn should follow since it needs `cmd.exe`?
- If user passes `options.shell=true` could be easily supported, but since the args passed are already escaped, we need to bypass the escaping and escape command in case of shebangs, see: https://github.com/IndigoUnited/node-cross-spawn/blob/master/lib/parse.js#L107. 
- If user passes `options.shell=string`, then he is trying to do something fancy and not so cross platform..

I think that, after weighting things, we should error out if user passed `options.shell=truish|false`.
What's your opinion @timdp?

@username_0 why are you using the `shell` option in the example above? You don't need to.
comment
created
johanneswuerbach
username_0
I need the shell option so commands like `spawn('coffee', ['-c', '*.coffee'])` are correctly expanded  in https://github.com/testem/testem/pull/998 on unix platforms.

Another option might be to unset `shell` when the platform is windows and to remove the escaping already in testem or to use plain windows node spawn when the option is set 🤔
comment
created
satazor
username_1
@username_0 I see. Well we can certainly support case `2` and `3`. For case `1`, we should error out saying we don't support it because we need to use `cmd.exe`.
comment
created
johanneswuerbach
username_0
That would definitely work for me. 👍
comment
created
satazor
username_1
@username_0 alright I'm going to work on this asap. I'm feeling very tired these days, working too much, so bare with me.
comment
created
johanneswuerbach
username_0
No worries, if there is anything I can help with, just ping :-)

Can also refine https://github.com/IndigoUnited/node-cross-spawn/pull/43 if you give me any hints / more tests cases.
comment
created
satazor
username_1
#43 is a great start, but it needs to be refined. The desired behavior looks like this:

If `options.shell === false`, then fail with a "not supported message".
If `options.shell` is truish, then bypass any escaping; if a shebang is detected, the arg pushed needs to be escaped.
If `options.shell` is null, then it basically is the current behavior.

Tests that need to be added:
   - The test added in #43 
   - Test that shebang works with `shell:true` and also test if the arg pushed is correctly escaped

Let me know if you want to work on this, otherwise I will pick it up when I get some time.
comment
created
satazor
username_1
@username_0 are you sure the windows shell support wildcard expansion? I think that, in your example, is actually "coffee" that is expanding the files, not the shell. See: http://superuser.com/questions/460598/is-there-any-way-to-get-the-windows-cmd-shell-to-expand-wildcard-paths
comment
created
johanneswuerbach
username_0
Correct, the windows shell doesn't support this. My use-case was more to allow running the same command (like the provided node script) consistently across all OSes.

Some commands testem receives are already escaped and were previously executed using exec, which has a limited output buffer size. I'm currently in the process of migrating all calls to use spawn with or without the shell option (depending whether exec was use previously).

So my use case was more to make cross-spawn more consistently work as a drop in replacement and not really about the shell features as those are shell dependent anyway. Does that make sense?
comment
created
johanneswuerbach
username_0
Another idea might be to just forward when shell: true and isWin to the default node implementation?
comment
created
satazor
username_1
I like that idea. We need to make that clear in the Readme. Time for a new PR? :p
comment
created
satazor
username_1
@username_0 can you take a look on https://github.com/IndigoUnited/node-cross-spawn/pull/45?
comment
created
johanneswuerbach
username_0
Also modified https://github.com/IndigoUnited/node-cross-spawn/pull/43, but having trouble to make the test pass.
comment
created
satazor
username_1
@username_0 I've worked a bit more and I came up with #46. It adds support for the `shell` option, even for node versions < 6. That PR also refactor the code to make it more clear.

Tell me what you think.
comment
closed
satazor
username_1
issue
created
satazor
username_1
@username_0 #46 landed in `v5.0.0`.
comment