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

Add compabitility with WSL #28

Merged
merged 1 commit into from
Aug 12, 2018
Merged

Add compabitility with WSL #28

merged 1 commit into from
Aug 12, 2018

Conversation

thijsputman
Copy link
Contributor

WSL (Windows Subsystem for Linux) presents itself as "Linux" in Node, but in order for a desktop application to be opened, requires the Windows-approach (i.e. calling cmd.exe).

This pull-request aims to detect this (peculiar? 😉) situation by looking at the value of os.release() and in that case following the Windows-approach. This required some minor modifications to the overall flow of the code.

Tested on physical Windows, Windows+WSL, Mac and Linux systems.

@domenic
Copy link
Owner

domenic commented Jul 30, 2018

Hmm, interesting!!

The thing I'd worry about here is what about people using opener in WSL to open Linux binaries and the like? I guess it'll probably still work?

@thijsputman
Copy link
Contributor Author

Fair point; that wouldn't work anymore (they'll be piped through to cmd.exe which doesn't know anything about the Linux environment the call came from).

On the other hand: On Linux you're using xdg-open, which - as far as I know - is intended to open a file/URL via the default application in the X11 system (i.e. open desktop application). That'll never work on WSL to begin with (as there's no formal X11 support; you could hack it together, but Microsoft's official stated purpose of WSL is to run command-line tools).

@domenic
Copy link
Owner

domenic commented Jul 30, 2018

That's a solid point, thanks! I'm convinced.

This has style issues that I need to clean up before merging. My bad for having such out of date lint technology. But I'll try to do that soon (and add eslint) and then merge and release.

@thijsputman
Copy link
Contributor Author

Perfect; thanks a lot!

@domenic domenic merged commit 11e582c into domenic:master Aug 12, 2018
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