Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save platforms and plugins to devDependencies #64

Closed
GedasGa opened this issue May 14, 2019 · 5 comments · Fixed by #65
Closed

Save platforms and plugins to devDependencies #64

GedasGa opened this issue May 14, 2019 · 5 comments · Fixed by #65

Comments

@GedasGa
Copy link
Contributor

GedasGa commented May 14, 2019

Feature Request

Motivation Behind Feature

Currently, Cordova platforms and plugins are saved as dependencies in the package.json file. Hence, during the Cordova prepare step, necessary files for plugins are moved for bundling with the app. These dependencies are not used directly with the app, so I believe, they should be treated as devDependencies.

I want to clear out Cordova platforms and plugins from dependencies because it could affect platforms that leverage the npm modules. This issues can be seen with the newest platform - Electron. With Electron it is expected that you use npm packages within the application and should be installed as dependencies.

For instance, if we want to use lodash with Electron app, we would npm install lodash and then import it using require. When we run the application everything works as expected because it uses the module from {project}/node_modules. But with build we need to take all the dependencies from the project and install them within the app.

As a result:

  • With Cordova platforms and plugins defined in the dependencies, for example, cordova-electron the packaged app size will increase greatly.

  • With Cordova platforms and plugins defined in the devDependencies, they would not be installed with the app and reduce the package size.

Again, Cordova platforms and plugins should be treated as development building blocks and shouldn't be in the app

Feature Description

The current implementation of the npm arguments handling is the following:

function npmArgs (target, userOptions) {
    const opts = Object.assign({ production: true }, userOptions);

    const args = ['install', target];

    if (opts.production) {
        args.push('--production');
    }
    if (opts.save_exact) {
        args.push('--save-exact');
    } else if (opts.save) {
        args.push('--save');
    } else {
        args.push('--no-save');
    }
    return args;
}

My suggestion would be instead of using --save flag, change it to --save-dev. While the --save option still appears to work, it is no longer required. The npm install saves any specified packages into dependencies by default.

Besides that, we also append --production flag. With the --production flag npm will not install modules listed in devDependencies. However, the --production flag has no particular meaning when we are adding a dependency to a project. I think that should be removed. Plus, if you use --production and --save-dev flags together, it causes npm install to break.

Later we could also remove options, like production, that get passed from cordova-lib to the cordova-fetch and do a general clean up.

Alternatives or Workarounds

Manually move Cordova platforms and plugins, that don't need to be bundled with an app, from dependencies to the devDependecies.

@raphinesse
Copy link
Contributor

👍

I'd say this change requires a major release of cordova-fetch. Given that and the fact that --no-save is broken anyway, we could seize the opportunity and remove support for any user-provided options.

@brodybits
Copy link

And I think a PR would be very welcome.

@GedasGa
Copy link
Contributor Author

GedasGa commented Jun 13, 2019

👍

I'd say this change requires a major release of cordova-fetch. Given that and the fact that --no-save is broken anyway, we could seize the opportunity and remove support for any user-provided options.

Yes, I also think that's for the next major release of cordova-fetch. In that case, maybe it would be possible to get rid of the cordova-fetch altogether. Anyway, I'll create a draft PR on the changes, I've implied.

@brodybits
Copy link

Thanks @GedasGa for your efforts. Getting rid of cordova-fetch sounds like a more drastic overhaul, would be nice if this could give us some performance benefits. I personally would not mind a gradual approach. PR #65 seems to be ready for review, not sure if that was your intention or not.

@GedasGa
Copy link
Contributor Author

GedasGa commented Jun 13, 2019

Yes, I did the changes that I have described. I believe that's all we need for saving platforms and plugins to the devDependencies.

However, I didn't remove the user-provided options as @raphinesse suggested, but I thought maybe that needs a further discussion and could be a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants