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

CB-14263: (android) make cordova plugin compatible with cordova-android 7 #35

Closed
wants to merge 2 commits into from

Conversation

leo6104
Copy link

@leo6104 leo6104 commented Aug 8, 2018

Platforms affected

cordova-android

What does this PR do?

My PR supports alias path in cordova-plugin config-file tag.
It will makes older cordova plugins's android config-file target compatible with cordova-android 7.

There is lots of problem with cordova-android 7 & cordova plugins. Many of cordova plugins still not upgraded to cordova-android 7 and there is limitation to wait for them to update their plugin.xml. So i think we need

What testing has been done on this change?

I create cordova-android 7 project with install cordova-plugin-device (only cordova-android <=6 support plugin)
and install via npm i -g https://github.com/leo6104/cordova-cli.git
and run cordova build android
and check platforms/android/app/src/main/res/xml/config.xml to correctly inject tag

I also test this commit to my production hybrid app (30k~40k user scale DAU app - 14 cordova plugins)

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

function _parseConfigFile (tag) {
function _parseConfigFile (platform, tag) {
var target = tag.attrib['target'];
if (platform == 'android') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're currently trying to remove all platform specific code from cordova-common, so this might be a problem.

@raphinesse
Copy link
Contributor

@jcesarmobile didn't you fix this in cordova-android?

@janpio
Copy link
Member

janpio commented Aug 8, 2018

Hey @leo6104, ignoring what @raphinesse said - thanks for taking the time to create this! 🥇 I fully agree that some solution for this problem is badly needed. Let's see what @jcesarmobile says if this will be needed at all.

@raphinesse
Copy link
Contributor

This is the change I was talking about: apache/cordova-android#449

Does this address the same problem?

@leo6104
Copy link
Author

leo6104 commented Aug 8, 2018

@janpio @raphinesse Thank you for quick comment :) I agree with the point that if there is any platform-specific code should be implemented to cordova-android not cordova-common, i can do that and i will. But i didn't know what cordova-common plan before upload this issue because ConfigParser already have platform-specific code. :)

I see the cordova-android issue and i found that in pluginHandlers, there is no config-file handler. So i think these compatibility issue still exists on config-file tag use cases.
https://github.com/jcesarmobile/cordova-android/blob/master/bin/templates/cordova/lib/pluginHandlers.js

[Existing Problem]
When plugin.xml have <config-file target="AndroidManifest.xml" parent="/*">, it silently skipped. but when i run android emulator, only alert 'Error cordova initializing ~~' because of AndroidManifest.xml & res/xml/config.xml skipped info.
Is there any code to alias config-file tag cases?
If already fix it, i will check cordova-android master branch to my production hybrid app. :)

@jcesarmobile
Copy link
Member

jcesarmobile commented Aug 8, 2018

I agree, I think we shouldn’t add platform specific code here. Also this will break old platforms on new cli, not sure what’s our support policy for older platforms on newer clips though.

Also, I think this kind of problems should be fixed already in Cordova-android. Can you add cordova-Android from master or 7.1.x branch and see if the problem persists? And in case it persists, can you try to fix it there if possible?

BTW, this is the PR that should have fixed the problem, as the real problem was cordova-Android not detecting properly the type of project, so it looked for the AndroidManifest.xml and some other files in the wrong location
apache/cordova-android#437

@leo6104
Copy link
Author

leo6104 commented Aug 9, 2018

@jcesarmobile cordova-android master or 7.1.x branch dosn't work well.
I tried to fix it but i face some point it could be breaking changes.
https://github.com/apache/cordova-android/blob/59018ab632e99b3e45a0bf9657c70282aeeac7bf/bin/templates/cordova/lib/prepare.js#L40

In prepare process in cordova android, there is no code to control target path in config-file tag for plugins. cordova-android just call cordova-common's PluginMunger to process plugin installation.

ConfigChanges in cordova-common only do some plugin apply process.
https://github.com/apache/cordova-common/blob/master/src/ConfigChanges/ConfigChanges.js#L419

My thought about this issue (with minimum code changes in cordova-android)

  1. Extend PluginInfoProvider in cordova-android
  2. Implement get method and check the target exists or not. if not exists, append prefix path 'app/src/main' to the target path.
  3. It will make https://github.com/apache/cordova-common/blob/master/src/ConfigChanges/ConfigChanges.js#L413 this get method line work.

Is it okay? or any ideas to fix it?

@leo6104
Copy link
Author

leo6104 commented Aug 9, 2018

I found strange behavior.

For test, i remove all platforms & plugins folder in my project and run cordova platform add android@7.1 and watch platforms/android/app/src/main/res/xml/config.xml file while processing platform add.

In platforms/android/app/src/main/res/xml/config.xml, while Discovering plugins & adding progress, it seems merge well. but after the command cordova platform add finish, platforms/android/app/src/main/res/xml/config.xml only have some plugins which correctly define config.xml path in their plugin.xml.

com.telerik.plugins.nativepagetransitions
com-sarriaroman-photoviewer
cordova-plugin-android-permissions
cordova-plugin-customurlscheme
cordova-plugin-device
cordova-plugin-dialogs
cordova-plugin-enable-multidex
cordova-plugin-facebook4
cordova-plugin-file
cordova-plugin-file-opener2
cordova-plugin-igaworks
cordova-plugin-inapppurchase
cordova-plugin-ionic-webview
cordova-plugin-keyboard
cordova-plugin-nativestorage
cordova-plugin-naver
cordova-plugin-splashscreen
cordova-plugin-statusbar
cordova-plugin-whitelist
cordova-plugin-x-socialsharing
cordova-plugin-x-toast
cordova-support-google-services
es6-promise-plugin
phonegap-plugin-multidex

Here is my plugin list. all of plugin version is latest

@leo6104
Copy link
Author

leo6104 commented Aug 11, 2018

After i remove shell.cp('-f', locations.defaultConfigXml, locations.configXml); in restore-util.ts file,
it work.

Working version

  1. cordova 8.0
  2. cordova-android https://github.com/apache/cordova-android

@janpio
Copy link
Member

janpio commented Sep 5, 2018

I fixed an Javascript problem eslint was complaining about. Now the tests complain about real test failures Can you take a look please @leo6104? Thanks.

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

We have applied some updates to 7.1.x and master branches of cordova-android that should resolve the issues with XML files for plugins that have not been updated for Cordova 7. In case we encounter any more issues with plugin XML files I would rather we update cordova-android than this common package.

@dpogue dpogue closed this Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants