Always use where command provided by Windows

[JPanneel]

#69

By using the absolute path to the where executable provided by Windows
we avoid issues with other variants of the where command on the user's
path.

C:\Windows\System32\where.exe can be used to search a directory:
where /r

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[orta]

This looks and feels good to me, can you please add a note in the CHANGELOG.md about the fix - then we should be good to go.

@gabelevi - think you can do a quick look over this (as you're a windows user I believe) and then ship a release?

[orta]

Thanks for the PR @JPanneel :D

[gabelevi]

Awesome, thanks for fixing this @JPanneel!

I'm afraid I'm travelling without my Windows laptop this week and I don't have enough space to spin up a Windows VM. The code looks good to me when I read it, but ideally someone would test the plugin on Windows before I deploy.

@JPanneel - What was your test plan? @orta - would you mind if I relied on @JPanneel's Windows testing and just tested on OSX before releasing?

[JPanneel]

@gabelevi
I tested by just logging the executed where commands and testing them directly in cmd.
I could go through the whole flow startup phase for global and local install and check how it works with a configured path to flow when that contains a directory as well. Let me do this in the evening and I will report back here before merging.

I can only test on Windows 10 though, but then again C:\Windows\System32\where has been there for quite some time...

[orta]

Yeah, this seems 👍 to me

[JPanneel]

I tested 4 configurations. 1 and 2 work as intended, 3 and 4 have 2 issues:

1. "flow.useNPMPackagedFlow": true results in
   C:\Windows\System32\where /r c:\Users\jensp\development\Project\node_modules\.bin flow.cmd
   which finds flow if it is there. (This was not working for me before)
2. "flow.useNPMPackagedFlow": false && "flow.pathToFlow": "flow" results in
   C:\Windows\System32\where flow
   which finds flow if it is on the path i.e. if it is executable as flow from the command line. (this is the default configuration)
3. "flow.useNPMPackagedFlow": false && "flow.pathToFlow": "<some_absolute_path_to_flow>" results in
   C:\Windows\System32\where /r some_absolute_path_to_dir flow
   which finds flow if it is in the specified location.
4. "flow.useNPMPackagedFlow": false && "flow.pathToFlow": "<some_relative_path_to_flow>" results in
   C:\Windows\System32\where /r some_relative_path_to_dir flow
   which finds flow if it is in the specified location.

Issues:

1. Issue with configuration 4 (use of relative path) This is not very intuitive and almost never what you want as the current directory is not the project folder but rather the installation path of vs code.
   Suggested approach: add code to differentiate between absolute and relative path (absence of a colon) and prefix the relative path with global.vscode.workspace.rootPath.
2. Issue with both configuration 3 and 4: The user needs to use escaped backslashes (\\) in order for Windows to recognise it as a path.
   Suggested approach: add this info in the settings description. (cleaner than the error prone substitution of slashes.)

[orta]

Suggested approach: add this info in the settings description. (cleaner than the error prone substitution of slashes.)

Agreed

[JPanneel]

I added the text "On Windows use '\' as directory separator" to the settings description.

What about?

Suggested approach: add code to differentiate between absolute and relative path (absence of a colon) and prefix the relative path with global.vscode.workspace.rootPath.

Do relative paths even make sense in this situation? Maybe something for a separate pull request as this was about useNPMPackagesFlow...

[orta]

I initially thought that the path option would handle relative paths, so I'm in favour of the idea, but happy for it to move to another PR

[JPanneel]

Ok, so: Works on my machine! (except for relative paths) Would be nice if someone else could confirm though...

[robarnold]

I think there's no need to have the user escape the backslashes if you wrap the directory in double quotes when passing it to where. Ex: where.exe /R "C:\Users\robarnold\AppData\Roaming\npm" "flow.cmd"

This should also allow users to provide paths that have spaces in them on Windows.

[JPanneel]

When passing the command to where, it actually contains only single backslashes. I think it is while reading it from the settings file into the config variable that the backslash is interpreted as an escape character.

[orta]

Hrm, I'd rather this not sit idle - @JPanneel wanna give it one more look over and then if you give it the 👍 - I'm good to go

[gabelevi]

Ok, I tested this out on my Windows machine. I made a small change to getPathToFlow() to log which path to flow is in use. Then I tested

• flow.useNPMPackagedFlow: true without a flow-bin installed resolves to the flow.pathToFlow
• flow.useNPMPackagedFlow: true witha flow-bin installed resolves to the node_modules/.bin/flow.cmd
• flow.useNPMPackagedFlow: false resolves to the flow.pathToFlow

In each case, this extension seemed to work. So this seems good to merge!

I think that little bit of logging is useful, so I'll commit it separately.