Open ashleydavis opened 5 years ago
Thanks for the issue - this seems like a valid use case and I've started looking into it.
Thanks so much for your help. Let me know if there's anyway I can assist.
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?
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:
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:
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).
Hi @ashleydavis and thanks for the test!
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.
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!
@nathanhleung I've committed the new test and code to make it pass.
https://github.com/ashleydavis/install-peerdeps/commit/95dd3d1a982f1d4f36c34c6703754c934a389069
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!
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!
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.
No worries, thank you for your help so far and I'll keep this open in case you want to come back to it!
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 ofinstall-peerdeps
within Data-Forge Notebook to automatically install peer dependencies like this. What it does in this case is runnpm install request-promise
and then it runsinstall-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:
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:
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.