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

Remove saving platforms/plugins to config.xml #750

Merged
merged 1 commit into from Mar 15, 2019

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Mar 14, 2019

Platforms affected

All (via tooling)

Motivation and Context

We've had one major version of cordova-lib that attempted to ensure both config.xml and package.json were kept in sync, and now it's time to push people more strongly towards using package.json.

Description

We will still attempt to read values from config.xml and update them in package.json, but we will no longer reflect changes back to config.xml.
Whatever's saved in package.json should always have priority over what is read from config.xml.

The next phase will be to improve the handling of package.json updates, and then in the next major to completely remove the code that looks at config.xml for platforms/plugins.

Testing

Updated spec and E2E tests.

Checklist

  • I've run the tests to see all new and existing tests pass

We've had one major version of cordova-lib that attempted to ensure both
config.xml and package.json were kept in sync, and now it's time to push
people more strongly towards using package.json.

We will still attempt to read values from config.xml and update them in
package.json, but we will no longer reflect changes back to config.xml.
Whatever's saved in package.json should always have priority over what
is read from config.xml.

The next phase will be to improve the handling of package.json updates,
and then in the next major to completely remove the code that looks at
config.xml for platforms/plugins.
@dpogue dpogue requested review from raphinesse and erisu Mar 14, 2019
@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Mar 14, 2019
@codecov-io
Copy link

codecov-io commented Mar 14, 2019

Codecov Report

Merging #750 into master will decrease coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #750      +/-   ##
=========================================
- Coverage   86.04%   85.8%   -0.24%     
=========================================
  Files          50      50              
  Lines        2666    2628      -38     
=========================================
- Hits         2294    2255      -39     
- Misses        372     373       +1
Impacted Files Coverage Δ
src/cordova/plugin/add.js 98.23% <ø> (-0.05%) ⬇️
src/cordova/platform/addHelper.js 95.56% <ø> (-0.75%) ⬇️
src/cordova/restore-util.js 94.5% <ø> (-0.76%) ⬇️
src/cordova/platform/remove.js 100% <100%> (ø) ⬆️
src/cordova/plugin/remove.js 97.33% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 733b3a4...d9d36f1. Read the comment docs.

Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to ✅ Approved, waiting for Merge Mar 15, 2019
Copy link
Contributor

@raphinesse raphinesse left a comment

LGTM

events.emit('log', 'Removing platform ' + target + ' from config.xml file...');
cfg.removeEngine(platformName);
cfg.write();
if (cfg.getEngines && cfg.getEngines().some(function (e) { return e.name === platformName; })) {
Copy link
Contributor

@raphinesse raphinesse Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (cfg.getEngines && cfg.getEngines().some(function (e) { return e.name === platformName; })) {
if (cfg.getEngines().some(e => e.name === platformName)) {


// No pkg.json included in test file.
describe('local path is added to config.xml without pkg.json', function () {
beforeEach(() => useProject('basePkgJson13'));
Copy link
Contributor

@raphinesse raphinesse Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check if the fixture is still used now that this has been deleted? What is the new behavior without a package.json?

Copy link
Member Author

@dpogue dpogue Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not really look into the tests beyond trying to make them pass. 😓

My hope is to revisit these when we update the package.json handling.

Copy link
Contributor

@raphinesse raphinesse Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well basePkgJson13 refers to a project fixture that might be unused now. I can't check it now though.

erisu
erisu approved these changes Mar 15, 2019
Copy link
Member

@erisu erisu left a comment

LGTM

@erisu erisu merged commit 8371d7c into apache:master Mar 15, 2019
Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Mar 15, 2019
dpogue added a commit to dpogue/cordova-lib that referenced this pull request Mar 22, 2019
Some of the code removed in apacheGH-750 had side effects on the rest of the
plugin restoration procedure. Namely, despite wanting to migrate to
package.json, we actually relied on config.xml for retrieving the plugin
spec and variables. When we stopped syncing changes back to config.xml,
those plugins stopped getting restored.

I've refactored this significantly to reduce complexity and make it
easier to read and understand. It appears to work properly, having been
tested in several of my own apps now, but I don't trust that the
unit/e2e tests actually validate it, so I encourage futher testing.
dpogue added a commit to dpogue/cordova-lib that referenced this pull request Mar 22, 2019
Some of the code removed in apacheGH-750 had side effects on the rest of the
plugin restoration procedure. Namely, despite wanting to migrate to
package.json, we actually relied on config.xml for retrieving the plugin
spec and variables. When we stopped syncing changes back to config.xml,
those plugins stopped getting restored.

I've refactored this significantly to reduce complexity and make it
easier to read and understand. It appears to work properly, having been
tested in several of my own apps now, but I don't trust that the
unit/e2e tests actually validate it, so I encourage futher testing.
dpogue added a commit that referenced this pull request Mar 29, 2019
Some of the code removed in GH-750 had side effects on the rest of the
plugin restoration procedure. Namely, despite wanting to migrate to
package.json, we actually relied on config.xml for retrieving the plugin
spec and variables. When we stopped syncing changes back to config.xml,
those plugins stopped getting restored.

I've refactored this significantly to reduce complexity and make it
easier to read and understand.

The existing tests did neither describe nor test the new behavior
sufficiently.
@jpike88
Copy link

jpike88 commented Apr 4, 2019

Should at least put a warning in the CLI! This is a huge change and was throwing me off for an hour while I was figuring out why plugins mysteriously were disappearing from config.xml.

@dpogue
Copy link
Member Author

dpogue commented Apr 4, 2019

@jpike88 Nothing should be disappearing from config.xml, we're not removing anything from the file. We're just not adding to it anymore, and instead consolidating everything in package.json.

@dpogue dpogue deleted the configxml-saving branch Apr 4, 2019
@jpike88
Copy link

jpike88 commented Apr 4, 2019

I think a warning that's detecting existing mentions of plugins/engines in config.xml should be thrown, because tools like cordova-check-plugins rely on a rm/add process through Cordova and it can be disorienting to see things just disappearing from config.xml. Would make more sense to encourage users to clear it out, or just do a full clear out with a message saying why.

@tfrijsewijk
Copy link

tfrijsewijk commented Jun 20, 2019

Help me out here.. I started a new cordova project, added some plugins, and now I want to build my project using PhoneGap Build. But PGB wants config.xml..

What to do? How will PGB know which plugins to restore if it doesn't know what to do with package.json?

@janpio
Copy link
Member

janpio commented Jun 26, 2019

That sounds like a problem with Phonegap Build @tfrijsewijk, not with cordova-lib. Please talk to the Phonegap Build people on how to solve this problem.

@tfrijsewijk
Copy link

tfrijsewijk commented Jun 26, 2019

You are right! But have you ever been to the Adobe Community forums? :-)

This is a pretty breaking change (hence the major increment). I came here through Google -> Stackoverflow (1st hit) -> Second answer and I can imagine I'm not the only one.

So - pretty please - can you give me a hint on how to get this going for PhoneGap Build? I mean, PhoneGap Build uses cordova right? Is this is a matter of patience and PGB will support plugins from package.json ?

@janpio
Copy link
Member

janpio commented Jun 26, 2019

I literally have no idea, as I have never used Phonegap Build and don't know how it works. I think they use a forked version of Cordova behind the scenes, but usually lag behind. From rereading your comments here, I would try to manually add the plugins to config.xml (which is still valid for Cordova CLI, just not the default and main way) and see if this fixes stuff for PGB.

(If you want to keep discussing this, better open a new issue here instead of commenting on a merged pull request. We usually don't like support requests like this, but abusing an old PR is even worse and more confusing)

@ryanmtaylor
Copy link

ryanmtaylor commented Jul 11, 2019

I'd love to see the fact that config.xml plugins don't really affect anything (and hence aren't being added) documented literally anywhere – eg. https://cordova.apache.org/docs/en/latest/config_ref/#plugin

This is the only place I could find really any documentation of this fact.

@janpio
Copy link
Member

janpio commented Jul 11, 2019

(Same for the fact that plugins come from package.json probably)

@janpio
Copy link
Member

janpio commented Jul 11, 2019

Best open a new issue for this @ryanmtaylor - otherwise it will get lost here in this closed PR. cordova-lib is probably an ok location, as the change was done here.

@raphinesse
Copy link
Contributor

raphinesse commented Jul 11, 2019

I'd argue that PRs and issues that aim to improve the documentation should be created in cordova-docs.

@ryanmtaylor
Copy link

ryanmtaylor commented Jul 11, 2019

@janpio @raphinesse thank you created the feature request here: apache/cordova-docs#1004

@pjc2007
Copy link

pjc2007 commented Nov 26, 2019

As per mention above (linked issue), I had to manually add the cordova-plugin-file plugin into config.xml for it to work. This stumped me for a very long time (as I never thought to try this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Apache Cordova: Tooling Pull Requests
🏆 Merged, waiting for Release
Development

Successfully merging this pull request may close these issues.

None yet

9 participants