cordova-lib icon indicating copy to clipboard operation
cordova-lib copied to clipboard

[cordova-cli #578] plugin/remove.js: don't stomp opts.cli_variables in removePluginFromPlatform()

Open sebastian-onlea opened this issue 2 years ago • 2 comments

Platforms affected

CLI - adding and removing plugins with differing required variables per platform (e.g. cordova-plugin-googleplus )

Motivation and Context

resolves apache/cordova-cli#578 : cordova plugin rm plugin-package --variable "IOS_ONLY_REQUIRED_VARIABLE=variable-value" will fail if the project includes e.g. both android and ios platforms.

Description

In removePluginFromPlatform(), at cordova/plugin/remove.js:105, opts.cli_variables is destructively reassigned to the result of mergeVariables(). As mergeVariables() selects only keys specified for a specific platform, this can result in CLI-specified values being lost before the platform that requires them is processed.

In the change, instead call uninstallPlatform() with a copy of opts that includes the platform-specific mergeVariables() result.

Testing

  • Ran existing tests (npm run test)
  • verified the change fixes my local use case by npm linking

Checklist

  • [x] I've run the tests to see all new and existing tests pass
  • [ ] I added automated test coverage as appropriate for this change

    I'm not familiar enough with how to create a reproduction case in the test library for this project.

  • [x] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • [x] If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • [x] I've updated the documentation if necessary

    No doc update should be required

sebastian-onlea avatar Mar 28 '23 20:03 sebastian-onlea

Thank you for the PR.

We generally do not make back ports releases. If applicable, the PR should target the master branch so that when merged it will be included in our next release. I'm not sure if GitHub allows you to re-target a PR, it may need to be closed and recreated.

breautek avatar Mar 29 '23 13:03 breautek

Will do! I've been working on the released version 11 and I wasn't sure whether to target the patch branch or master. Thanks.

sebastian-onlea avatar Mar 29 '23 15:03 sebastian-onlea

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.36%. Comparing base (7f8b2d0) to head (2a0993e). Report is 29 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #913   +/-   ##
=======================================
  Coverage   88.36%   88.36%           
=======================================
  Files          46       46           
  Lines        2123     2123           
=======================================
  Hits         1876     1876           
  Misses        247      247           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov-commenter avatar Jul 05 '25 14:07 codecov-commenter