Open raphinesse opened 6 years ago
npm assumes --save
by default as of npm@5. Is it an option to just remove all our { save: true }
code and let npm do what it does?
The point is, that we never call npm
if we detect that the requested package is already installed. Thus it does not matter what npm
does by default, since it won't do anything at all.
That being said, I would prefer if we did not have any options to pass to npm. I'm not sure it is feasible though. And I think that this is a separate topic.
I also think we should just remove this option and use npm's default behavior which automatically saves the dependency to the package.json.
Though with limited insight without actually trying to reproduce/test what your seeing, I don't understand what
The point is, that we never call npm if we detect that the requested package is already installed.
has to do with the {save:true}
. I don't think the save flag should have any affect towards the detection logic.
I think what Raphael is meaning is that it's "broken" in the following case:
# Installs it into node_modules without updating package.json
npm install --no-save cordova-plugin-camera
# This detects that it's already installed in node_modules and
# doesn't invoke npm at all, therefore it's never added to package.json
cordova plugin add cordova-plugin-camera
I'm not sure what to do in that particular edge case, aside from maybe documenting it?
How about something like this? Yes documentation is good like you said, but maybe also include user action feedback? Just would involve a bit of work.
$ cordova create project
$ cd project
$ npm install --no-save cordova-plugin-camera
$ cordova plugin add cordova-plugin-camera
> Cordova has discovered that plugin `cordova-plugin-camera` already exists but not saved. Missing configurations will be added to package.json. Cordova will not override or save the plugin to package.json. For more information: https://cordova.io/frequent-issues
or
$ cordova create project
$ cd project
$ cp -R ~/some-plugin ./node_modules/
$ cordova plugin add cordova-plugin-camera
> Cordova has discovered that plugin `cordova-plugin-camera` already exists but not saved. Missing configurations will be added to package.json. Cordova will not override or save the plugin to package.json. For more information: https://cordova.io/frequent-issues
I believe cordova plugin add
should still update package.json
with the plugin configurations if it does not today.
"cordova": {
"plugins": {
"cordova-plugin-camera": {}
}
}
$ cordova create project
$ cd project
$ npm install --no-save cordova-plugin-camera
$ vi package.json
-- I add manually the Cordova plugin configs
$ cordova plugin add cordova-plugin-camera
> Cordova has discovered that plugin `cordova-plugin-camera` already exists but not saved. Cordova will not override or save the plugin to package.json. For more information: https://cordova.io/frequent-issues
Only difference between these cases is that I had already provided Cordova plugin configurations in package.json. The feedback message drops about adding missing configs.
We can also create the frequent-issues page that would explain a bit more detail on how something like this happens. For example the npm i --no-save
.
Lastly, what if a plugin has a postinstall
script. Should we still trigger this on cordova plugin add
even though npm is not called.
Anyways, I understand now the use case.
After merging #24, we do not invoke
npm install
if the requested package is already installed. Thus,cordova-fetch
will not add the requested package todependencies
ofpackage.json
.However, some tests in
cordova-lib
expect that callingcordova-fetch
with{save: true}
will add the requested package todependencies
ofpackage.json
. This does not cause the tests to fail with the current version ofcordova-fetch
since there are no "dependency cache-hits" right now. But when I experimented with #44 to speed upcordova-lib
E2E tests, some of the tests started to fail.So how should we handle
save: true
when the requested package is already installed?I think the current behavior is fine. When we find a package already installed, either a user manually installed it, or it had been previously installed by Cordova. In any case, it would have been added to
dependencies
unless that behavior was explicitly suppressed. So I don't really expect this behavor to break anything.Frankly, it would be better if
cordova-fetch
would only fetch dependencies and was not expected to managepackage.json
in the first place, but that is a different story I guess.