Support Windows and npm link install scenario

[ayan4m1]

I would like to use this package to help manage the pain point of installing react-dom manually. This package would seem to be the perfect solution. However, I found (after several hours investigation) that it does not seem to support Windows, because the matcher regex used to find npm and/or yarn only matches on Unix-style paths.

In testing/reproducing this, I also found that it does not work when used as an npm link package - because __dirname used in install.js points to the physical path on disk of the non-symlinked package directory, not the path that install-peers occupies under the node_modules of the package (e.g. /code/install-peers rather than /code/install-peers-test/node_modules/install-peers which is a symlink to /code/install-peers).

I am using these configurations to test peer dependency installation.

Example output using 1.0.2 on Windows (react-dom is not present in node_modules afterward):

$ npm install

[email protected] postinstall C:\code\install-peers-test\node_modules\install-peers
node install.js

npm WARN [email protected] requires a peer of react-dom@^16.6.0 but none is installed. You must install peer dependencies yourself.
npm WARN [email protected] No description
npm WARN [email protected] No repository field.

added 10 packages from 4 contributors and audited 16 packages in 4.096s
found 0 vulnerabilities

Example output using my branch on Windows (react-dom is present in node_modules afterward):

$ npm install

[email protected] postinstall C:\code\install-peers-test\node_modules\install-peers
node install.js

npm WARN [email protected] No description
npm WARN [email protected] No repository field.

• [email protected]
  added 1 package and audited 16 packages in 51.138s
  found 0 vulnerabilities

Installed peerDependencies as devDependencies via npm.
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN [email protected] requires a peer of react-dom@^16.6.0 but none is installed. You must install peer dependencies yourself.
npm WARN [email protected] No description
npm WARN [email protected] No repository field.

added 10 packages from 4 contributors and audited 16 packages in 70.714s
found 0 vulnerabilities

I have prepared this pull request with four changes:

• Added code to install-yarn.js and install-npm.js that "flips" the path separators if the platform is win32, allowing the regex matcher to work on Windows
• rootPath will be set to process.env.INIT_CWD if it is present (npm 5+), which allows the package to be used in npm link scenarios on *nix - this environment variable avoids the need to use relative pathing to find the package root
• Changed __dirname to process.cwd() in rootPath fallback initialization, which allows the package to be used in npm link scenarios
• Added a console.error message if neither yarn nor npm are found on the system, thus alerting the user to the reason no action was taken during execution

There is one case, where the user is on *nix and does not have npm 5+, where the npm link scenario will fail. However, this has always been the case. Unfortunately, process.cwd() process.env.PWD and __dirname are all the "incorrect" value in this environment, and process.env.INIT_CWD is not available.

My testing on Windows was done using Windows 10, Node 10.12.0. I regression-tested the package using it in both npm link and non-link scenarios on CentOS 7, Node 6.14.4 (npm 3) and 10.13.0 (npm 6).

Please let me know your thoughts and feedback.

[alexindigo]

Hey @ayan4m1, thanks for your interest in the package.
And windows support will definitely help.

Let me go over your changes to better understand it.

In the same time, I've found that it's easier and seems to be more stable to just use cli "interface" for npm (similar way how it's done for yarn) rather than trying to keep up with internal changes.

This is how I'd like to do it in this package as well:
https://github.com/alexindigo/install-peers-cli/blob/master/install-npm.js#L9-L29

Please review that part to see if it matches your use case and how it would affect your changes in this PR.

Thank you.

[ayan4m1]

Thank you for the quick feedback! I have hopefully addressed everything you pointed out.

• Instead of mangling the env var, use path.join as suggested combined with slightly simpler string comparison
• Changed install-npm to use the executioner style invocation (taken from install-peers-cli)
• Added console.log of output for both npm and yarn invocations via executioner (taken from install-peers-cli)
• Wrap ${node} and ${npm/yarn} in executioner string in double-quotes because the path can/will have un-escaped space(s) on Windows

It is worth noting that with Yarn, the console.log output is being hidden or lost due to yarnpkg/yarn#5476 - no idea what's going on there but it threw me for a loop at first. Theoretically this code will work if and when the yarn behavior changes.

Re-tested on Windows Node 10, Linux Node 6 and 10, yarn and npm - all scenarios result in peer dependencies correctly installed.

[alexindigo]

Thanks a lot for the changes. I'll add your as a contributor, so you can merge when it's all clear.
And what is your npm handle, I'll add you to the project, so you can publish updated versions.

Thank you.

[ayan4m1]

Happy to help, thanks for being so responsive. I am also ayan4m1 on npmjs.com. I will get some friends to test on Macs and in some virtualized environments, but ultimately I'd like to release this as 1.0.3 if that makes sense to you.

[alexindigo]

Perfect. Thank you.

Added you on npm:

$ npm owner add ayan4m1 install-peers
+ ayan4m1 (install-peers)