search

mattludwigs / elm-version-manager

Manage local Elm versions: https://www.npmjs.com/package/elm-version-manager
29 stars 2 forks source link

Add Windows support #4

Closed trytriangles closed 7 years ago

trytriangles commented 7 years ago

1

This contribution adds Windows support. In my limited testing, it all seems to work fine using the npm-installed version of Elm — I can't say whether the dedicated installer from elm-lang.org has any clash with it, what with registry keys and all.

Windows lacks a direct equivalent to /usr/local/bin. Instead, when you use 0.18.0, 0.18.0's executables are copied into the %USERPROFILE\AppData\Local\Elm Version Manager folder for the current user. Because Windows doesn't allow non-administrators to create symlinks, that's a direct file copy overwriting what was previously in use.

There's one thing necessary for use that I haven't done: added that folder to the %PATH% variable. Users will need to manually add %USERPROFILE\AppData\Local\Elm Version Manager to their %PATH% using the Windows control panel (System → Advanced System Settings → Environment Variables) or the command prompt. I left this out because I wasn't sure an NPM package permanently altering your environment variables was something users would appreciate, might be better as a manual step detailed in the README. Could always add a Y/N option for that, though.

Someone, please let me know if there's a better way to implement this, I haven't done anything quite like it before and I'm totally expecting my solution to be naïve. Anyway, I'm on Windows for the moment, I needed an Elm version manager, I found this, I added support, and it's been working for my needs today.

trytriangles commented 7 years ago

Bonus commit: by hardcoding the 'elm-' prefix we can add the plain old elm binary for people who use that. elm-make, elm-package, etc being a different version than elm make and elm package tripped me up when I saw it.

mattludwigs commented 7 years ago

Awesome! Thank you so much for the PR @ryanplant-au! Also, just know you are in good company because I have not done anything like this before either, but I needed an Elm version manager too.

In regards to your thoughts on updating the path variable I would agree. For now you or I can add that to the readme for windows users. However, a better long term solution and user experience will be asking the user and trying to do it from the CLI. I would rather not be blocked by that since that would be a new feature, and would rather get this at least working on Windows first.

As an aside, thanks for the the spelling fixes too. I was working too fast and have missed some of those.

If you could make one small code update that would be great and help with refactoring later down on the road:

where you use if (process.platform === 'win32') you can just use operatingSystem in replace of the process.platform, as that variable is defined on line 19.

Other than that this looks good, I am going to give this a go locally to make sure everything still works on my Mac OS X (which I think it will). If so, I will get this and an updated README merged and pushed up to NPM soon!

Again thank you for taking this on!

mattludwigs commented 7 years ago

@ryanplant-au it is working as expected on my Mac, once that one update is done I can merge 👍.

If you feeling up to it you can add the Windows instructions to the README. If not I can get that added in before the next release.

And that bonus commit is awesome! I did not even think of that!

mattludwigs commented 7 years ago

@ryantriangles I am going to go ahead and merge this in and get it published on npm.