install-peerdeps overwrites existing module with an older version

[ashleydavis]

Hi thanks for this package, it's really useful.

I've noticed a (probably obscure) problem with it and just wondering if you have any advice on how to work around it.

First, some background. I'm working on a product called Data-Forge Notebook and it automatically installs npm modules that it finds used in scripts. I found it annoying that npm didn't automatically install peer dependencies. So that lead me to your package.

Here's an example of how I use it. A user of my product includes request-promise as follows:

const request = require('request-promise');

When a user runs this script in Data-Forge Notebook the module is automatically installed for them. This module also has a peer dependency on the request module. I'm made use of install-peerdeps within Data-Forge Notebook to automatically install peer dependencies like this. What it does in this case is run npm install request-promise and then it runs install-peerdeps request-promise --only-peers --silent. This works really well in this case and it ensures that my users automatically have the npm modules they need and also the peer dependencies.

However I discovered a problem with a more complicated use case.

You can easily try this example for yourself to see the result.

Create a new directory and package file:

mkdir test
cd test
npm init -y

Now install Data-Forge:

npm install --save data-forge

This installs the latest version of Data-Forge which is currently 1.3.0.

Now install the Data-Forge add on library data-forge-indicators.

npm install --save data-forge-indicators

data-forge-indicators has a peer dependency of data-forge.

Now run your tool as follows:

npm install -g install-peerdeps
install-peerdeps data-forge-indicators --only-peers --silent

This actually replaces data-forge 1.3.0 with the older version 1.0.10.

data-forge-indicators has a peer dependency on data-forge at or above that version, yet install-peerdeps replaces the existing version with the older version.

I'm thinking that install-peerdeps should be a bit smarter and realise that there is already a new version installed and that it doesn't need to actually do any work.

What do you think? Is this the way install-peerdeps is supposed to work or is this a bug or maybe a use case that hasn't been thought of?

Thanks for your package and your help.

[nathanhleung]

Thanks for the issue - this seems like a valid use case and I've started looking into it.

[ashleydavis]

Thanks so much for your help. Let me know if there's anyway I can assist.

[ashleydavis]

Any progress on this issue? Is there anyway I can help?

I'd like to update the peer dependencies for my library data-forge-indicators, although then you'll lose my test case. Will that be a problem or do you want me to hold out from updating it a bit longer?

[nathanhleung]

Hi, thanks for reaching out again. I haven't been able to work on it recently - if you could submit a pull I'll gladly accept it.

One potential solution could be reading the package.json in __dirname (i.e. location where install-peerdeps is being run) and comparing the package.json version to the peerdep requirement using semver.

Other thoughts:

• If one version cannot satisfy different constraints, prompt user for desired version

[ashleydavis]

I've started having a look into this problem. I've written a test (added to cli.test.js) that replicates the problem, and @nathanhleung I'd like your thoughts on it:

test("peer dependency is not installed when a later version already exists", t => {
  //TODO: Remove existing node_modules under fixture "has-newer-peer-dep".
  //TODO: Do an npm install "has-newer-peer-dep".
  const cli = spawnCli("data-forge-indicators", "has-newer-peer-dep");
  cli.on("exit", code => {
    fs.readFile(
      "fixtures/has-newer-peer-dep/node_modules/data-forge/package.json",
      "utf8",
      (err, packageFileJson) => {
        if (err) {
          t.fail((err && err.stack) || err);
          t.end();
          return;
        }

        t.equal(code, 0, `errored, exit code was ${code}`);

        const packageFile = JSON.parse(packageFileJson);
        t.equal(packageFile.version, "1.3.3", "Bad version!");
        t.end();
      }
    );
  });
});

Some problems:

1. There is currently some manual setup required to run the test. Before running the test I change directory into the new fixture directory has-newer-peer-dep, then I delete the node_modules directory and do an npm install to get it into a known start state. Ideally this would be part of the test (see TODO comments above). Although it feels wrong. Should I be mocking this somehow instead of having the test interact directly with the file system?
2. I'm actually using data-forge and data-forge-indicators in the test. I did this because I wanted to be sure I was replicating the exact same problem that I'm having, although it feels a bit wrong to be working with real npm modules. Do you have any technique for mocking npm libraries for testing?

Once I've figured out the correct way to structure this test it shouldn't be too hard too actually implement code to pass the test (I think writing the test is probably more difficult than the code for the actual feature).

[nathanhleung]

Hi @ashleydavis and thanks for the test!

1. The fixtures directly is there for the purposes of testing, so no worries about mocking the file system. One thing you can do, however, to automate the manual set-up, is use Node's native spawn imported from child_process and pass the cwd (like you did above) with the command rm -rf node_modules. You can run npm install via spawn as well.

2. That's fine, and all the better if you fix your own issue! Although from a philosophical standpoint we could argue that the tests should be pure, in my view the fact that the tool is inherently side-effect-ful (making network requests, writing to the filesystem, etc) means that if our tests have side effects and work with real-world, potentially changing data, that's OK too (of course, if data-forge is removed from NPM we have a problem, but we can easily fix it by choosing a different module).

Appreciate your work so far and hope my comments are clear. Let me know if you've got any questions!

[ashleydavis]

@nathanhleung I've committed the new test and code to make it pass.

ashleydavis@95dd3d1

Unfortunately I've broken several other tests and I'm having a hard time figuring out why! Do you mind having a look and maybe pointing me in the right direction?

Thanks!

[nathanhleung]

Thank you for your contributions so far, and apologies for the late reply!

I've created a pull request with your latest changes so they could run on Travis. It looks the tests that add the global and save flags are the ones that are failing. I'm not exactly sure why either, but I've added some comments to the pull request (#46) which might help.

Also, I'm not sure if you can add commits to #46 yourself since I created it. If that's the case, feel free to open a new pull request and I'll close the one I created.

Thanks again for your help!

[ashleydavis]

Hey, I think I'm going to park this for now. It's turned out to be a bigger job than I expected. I realised today that it's actually much simpler for me replicate the small portion of install-peerdeps taht that I need into my own project than it is to try and get my contribution to install-peerdeps working properly.

Thanks again for this library, sorry that I don't have more time to contribute to it right at the moment. Maybe I'll come back to it again the future.

[nathanhleung]

No worries, thank you for your help so far and I'll keep this open in case you want to come back to it!