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

feat(PluginInfo): pass through XML attributes #181

Merged

Conversation

TylerBreau
Copy link
Contributor

@TylerBreau TylerBreau commented Oct 19, 2022

Platforms affected

All

Motivation and Context

Closes #179 (superceeds #179)

Currently, adding attributes to the XML tags requires a lot of repos to update.

cordova-common to read the new XML tag.
cordova-lib and cordova cli need to update their dependencies.
People need to update cordova-cli to have the cordova-common update.

Each platform that wants to use the new attribute needs to be updated.
cordova-docs also needs to be updated.

Depending on the number of platforms that needs the update you're looking at 5 or more repos requiring an update, even when it's simply add a brand new attribute to an XML tag.

This PR updates the XML parsing code to passthrough all attributes.
Existing attributes are parsed in the same way there parsed before. New attributes will be passed through without any addition parsing.

The intention is to stop requiring cordova-common updates in order to add attributes to XML tags.

The benefit of this PR is that it removes the need to update 3 repos when modifying supported XML tags. Those 3 repos being cordova-common, cordova-lib, and cordova-cli.

Description

Copies all attributes from XML tags to the return object in the respective XML tag get functions.

Existing attributes are still parsed in the exact same way as before.
E.X framework attribute "embed" is still passed through isStrTrue.

I don't believe any cordova-docs updates are required.

Testing

I've added unit tests for XML passthrough.

I've ran npm test on cordova-common, cordova-lib, cordova-cli. lib and cli were updated with the cordova-common change.
I've ran these changes on my company's app.

An update in my company's attribute is requiring me to add an new attribute to the framework tag. I've tested adding a new attribute to cordova-ios with my company's app using these changes.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • 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)
  • I've updated the documentation if necessary

Sorry, something went wrong.

@dpogue
Copy link
Member

dpogue commented Oct 19, 2022

This looks good, and definitely helps make it easier for platforms to support new attributes in the future!

It would be good to add unit tests to show that custom attributes do, in fact, get passed through and returned by the methods after this change.

@TylerBreau
Copy link
Contributor Author

This looks good, and definitely helps make it easier for platforms to support new attributes in the future!

It would be good to add unit tests to show that custom attributes do, in fact, get passed through and returned by the methods after this change.

Yea I was just looking at the unit tests, seeing there's basically no detailed testing for the XML parsing.

@TylerBreau
Copy link
Contributor Author

TylerBreau commented Oct 19, 2022

Seems like it is not possible to support additional attributes on preferences.

describe('Preference', () => {
    it('Test 007: Preference supports xml passthrough', function () {
        const preferences = pluginPassthrough.getPreferences('android');
        console.log(preferences);
        expect(preferences.passthroughpref.anattrib).toBe('value');
    });
});


<preference name="passthroughpref" anattrib="value" />
<!-- <preference name="passthroughpref2" anattrib="value2" /> -->

{ name: 'passthroughpref', anattrib: 'value', PASSTHROUGHPREF: null }



<preference name="passthroughpref" anattrib="value" />
<preference name="passthroughpref2" anattrib="value2" />

{
  name: 'passthroughpref2',
  anattrib: 'value2',
  PASSTHROUGHPREF: null,
  PASSTHROUGHPREF2: null
}

@dpogue
Copy link
Member

dpogue commented Oct 19, 2022

yeah, it looks like preferences doesn't return an object per-preference tag, it returns an object of all the preferences as key-value pairs.

The preferences stuff is designed to be pretty strictly key/value, so maybe we don't need to handle extra attributes in that particular case.

@TylerBreau
Copy link
Contributor Author

TylerBreau commented Oct 19, 2022

yeah, it looks like preferences doesn't return an object per-preference tag, it returns an object of all the preferences as key-value pairs.

The preferences stuff is designed to be pretty strictly key/value, so maybe we don't need to handle extra attributes in that particular case.

I'm thinking XML passthrough won't deal with preferences. I'll create an issue (#182) for documentation and the potential flaw but for this PR i'll just be undoing the support and moving on. Probably leave a comment as well.

@TylerBreau TylerBreau force-pushed the passthrough-framework-attributes branch from 26680fc to e64368f Compare October 19, 2022 14:39
@TylerBreau TylerBreau marked this pull request as ready for review October 19, 2022 14:40
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

Code LTGM

@breautek breautek added this to the 5.0.0 milestone Oct 19, 2022
@breautek breautek requested a review from dpogue October 19, 2022 17:50
@breautek
Copy link
Contributor

Requested dpogue for review to give him a chance to followup on his suggestion.

Added this to the 5.x milestone, cause I don't think we are planning on doing a 4.1 release, but I don't think this is a breaking change so we could move this to a 4.1 feature release.

@dpogue
Copy link
Member

dpogue commented Oct 19, 2022

FWIW, I think config.xml is parsed in each platform using its own cordova-common dependency (rather than the one provided by cordova-lib), specifically to work around the issues of versioning in cordova-cli and using older CLI versions with newer platform versions

@TylerBreau
Copy link
Contributor Author

FWIW, I think config.xml is parsed in each platform using its own cordova-common dependency (rather than the one provided by cordova-lib), specifically to work around the issues of versioning in cordova-cli and using older CLI versions with newer platform versions

In my experience with apache/cordova-ios#1266, cordova-ios seems to rely on the cordova-cli when parsing plugin.xml.
cordova-ios unit tests seemed to rely on its own cordova-common dependency.
Not sure if config.xml is handled differently than plugin.xml.

@breautek
Copy link
Contributor

Talking with Erisu, and we decided to get this into a 4.1 release, so that we can patch up the other dependency PRs faster, rather than only including this in a major release.

@erisu erisu changed the title Updated PluginInfo to pass through XML attributes. feat(PluginInfo): pass through XML attributes Oct 25, 2022
@breautek breautek merged commit 837d300 into apache:master Oct 25, 2022
@breautek breautek deleted the passthrough-framework-attributes branch October 25, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants