Update index.js to support windows (untested!)

[christophmegusta]

i discovered this on a new windows tablet by accident. I tested with Git Bash for Windows. This might break Linux or other Windows shells so be cautious and give it some extra testing.

Changes:

• instead of spawning the shellwrapper (.sh file) directly i do now spawn the "bash" aka opts.shell (so you can specify own shell).
• using path.join for concatenating file paths

[jessetane]

Generally looks good although it's really two unrelated patches - one for fixing the path, and the other for picking the shell to use. As you can see the test failed, the second bit about the shell may need a little more work.

[christophmegusta]

actually both are part of the fix for Windows.

1. The path -> for correct operating system path resolution, windows uses backslash.
2. The shell must directly be spawned. Therefor the "shell extension".

[jessetane]

I get that but the changes themselves are not related to each other, and only one of them breaks the build.

[jessetane]

So, they should at least be different commits, but if they were sep PRs at least one could get merged right away.

[coveralls]

Coverage decreased (-3.8%) to 96.154% when pulling 54eacec on christophmegusta:patch-1 into 0e833f1 on jessetane:master.

[christophmegusta]

i removed path changes from patch. i suppose that one was anyways cosmetics and node probably translates "/" to "" on lower level functions like spawn.

the reason for broken test was supposedly the test itself as it does some counting and comparison on number of keys on env object.
previously it was using implicitly the "sh" shell and i specified the "bash" as default shell. bash might bring in different default env vars as "sh" does so defaulting to "sh" fixes it.

[jessetane]

The existing functionality on unix allows the process owner's shell to be used by default, which may or may not be sh - your change here overrides this. I think a better way would be to support the option only when passed.

[jessetane]

Also, the path fix is probably good to have, I wouldn't mind having that as a sep pr.

[jessetane]

another thing - generally it's good to squash your commits on prs so that the history is as legible as possible for others.

[jessetane]

of course it multiple commits make sense, you should include then, but not wip or cleanup commits

[jessetane]

travis stuff should also be updated - can you bring node js versions up to date and include windows if possible?

[jessetane]

Sorry to be such a pain but it looks like ppl are actually using this module so we should probably make sure we don't break it: https://www.npmjs.com/package/shell-source

[jessetane]

Just thinking about what the ideal pr might looks like: branch name would be maybe windows-support. First commit would be updates to travis that show the existing tests fail on windows. If the existing tests don't fail as expected, then next commit would be adding a test that does. After that, each fix required to make the test(s) pass would be its own commit, with an human readable explanatory message.

[christophmegusta]

i agree with your suggestions.

though after thinking about it, lets discard that pull request.
I dont have time to make this PR clean. As we are talking Its more complex than i originally thought. i started this PR with online git editor as a 3 line fix.
To make it clean we must check for default shell. Maybe also add some switches for windows behaviour. Also tests need to be adapted to ensure coverage. This adds a whole new level of complexity.
Maybe its better for now to update README.md and mark that 1 line change so people can fix it themselves or make it a branch and work on this later when demand for windows grows stronger.

[jessetane]

Fair enough. I suppose we can also just leave this PR open as an indicator that this is still unresolved? Maybe someone else will come along and fix it - I don't have the time right now to do a good job of it either.

[christophmegusta]

fine for me 👍