How to handle save: true when requested package is already installed

[raphinesse]

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 to dependencies of package.json.

However, some tests in cordova-lib expect that calling cordova-fetch with {save: true} will add the requested package to dependencies of package.json. This does not cause the tests to fail with the current version of cordova-fetch since there are no "dependency cache-hits" right now. But when I experimented with #44 to speed up cordova-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 manage package.json in the first place, but that is a different story I guess.

[dpogue]

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?

[raphinesse]

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.

[erisu]

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.

[dpogue]

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?

[erisu]

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.

Use Case 1

$ 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": {}
        }
    }

Use Case 2

$ 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.