install-peerdeps overwrites existing module with an older version

[nathanhleung]

install-peerdeps overwrites existing module with an older version

[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!