EddyVerbruggen / Custom-URL-scheme

:link: Launch your Cordova/PhoneGap app by a Custom URL scheme like mycoolapp://
1.03k stars 365 forks source link

Cordova-Android 7.0.0 support #264

Open NeoLSN opened 6 years ago

NeoLSN commented 6 years ago

https://cordova.apache.org/announcements/2017/12/04/cordova-android-7.0.0.html

<!-- An existing config.xml -->
<edit-config file="AndroidManifest.xml" target="/manifest/application" mode="merge">

<!-- needs to change to -->
<edit-config file="app/src/main/AndroidManifest.xml" target="/manifest/application" mode="merge">

It means

 <config-file target="AndroidManifest.xml" parent="/manifest/application/activity">

Need change to

 <config-file target="app/src/main/AndroidManifest.xml" parent="/manifest/application/activity">

Could this plugin release a new version for this change?

EddyVerbruggen commented 6 years ago

I love it when Cordova changes break all of my plugins :(((

Would you mind doing a PR? Cheers!

peterpeterparker commented 6 years ago

If I may, I would suggest to have a look, as I do, at https://github.com/jeduan/cordova-plugin-facebook4/pull/601 to see how the same issue will be solved in cordova-plugin-facebook4

In this related PR there are some discussions about how to handle in a good way this upgrade

jacobweber commented 6 years ago

+1

peterpeterparker commented 6 years ago

Actually I don't think this improvement is needed. Just tried with a blank project. I have added cordova-android@7.0.0 and then the current Custom-URL-Scheme plugin. Once both added, I could find the following in platforms/android/app/src/main/AndroidManifest.xml

 <intent-filter>
       <action android:name="android.intent.action.VIEW" />
        <category android:name="android.intent.category.DEFAULT" />
        <category android:name="android.intent.category.BROWSABLE" />
        <data android:scheme="mycoolapp" />
 </intent-filter>

Therefore I think no improvements are needed. Is that correct?

jacobweber commented 6 years ago

Well, it's weird. There's some bad interaction going on when you use edit-config in your config.xml along with this plugin. Try running cordova prepare on the following config.xml (using cordova-cli 8.0.0):

<?xml version='1.0' encoding='utf-8'?>
<widget id="com.test.cordovatest" version="1.0.0" xmlns="http://www.w3.org/ns/widgets" xmlns:cdv="http://cordova.apache.org/ns/1.0">
    <name>CordovaTest</name>
    <content src="index.html" />
    <plugin name="cordova-plugin-customurlscheme" spec="4.3.0">
        <variable name="URL_SCHEME" value="x-com-sample-cordovatest" />
    </plugin>
    <edit-config file="*-Info.plist" mode="merge" target="NSCameraUsageDescription">
        <string>foo</string>
    </edit-config>
    <engine name="android" spec="7.0.0" />
    <engine name="ios" spec="4.5.4" />
</widget>

You get an error at the end:

(node:71281) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: doc.find is not a function
(node:71281) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

If you remove cordova-plugin-customurlscheme, you don't get the error.

peterpeterparker commented 6 years ago

@jacobweber would you like to try the following:

<plugin name="cordova-plugin-customurlscheme" spec="^4.3.0">
    <variable name="URL_SCHEME" value="x-com-sample-cordovatest" />
    <variable name="ANDROID_SCHEME" value=" " />
    <variable name="ANDROID_HOST" value=" " />
    <variable name="ANDROID_PATHPREFIX" value="/" />
</plugin>

I've got both cordova-plugin-customurlscheme, cordova-android@7 and edit-configs in my config.xml (the NSCameraUsageDescription too) and it worked fine

jacobweber commented 6 years ago

Hmm, I get the same result with that. Here's my project (with res, node_modules, platforms, and plugins removed). I'm doing this on a Mac with NodeJS 8.4.0.

I created it using:

cordova create CordovaTest com.test.cordovatest CordovaTest
cd CordovaTest
cordova platform add android@7.0.0
cordova platform add ios@4.5.4
cordova plugin add cordova-plugin-customurlscheme@4.3.0 --variable URL_SCHEME=x-com-sample-cordovatest

then I manually added to config.xml:

    <edit-config file="*-Info.plist" mode="merge" target="NSCameraUsageDescription">
        <string>foo</string>
    </edit-config>

and ran cordova prepare. I got the same error. CordovaTest.zip

peterpeterparker commented 6 years ago

@jacobweber I didn't faced any problem building your project, no error.

After unzip in the project I did

npm install
cordova plugin add cordova-plugin-customurlscheme --variable URL_SCHEME=x-com-sample-cordovatest
cordova platform add android
cordova build android

what I could think of is

  1. What's your cordova version? Did you have cordova 8 installed?
  2. I used Node v9.3.0
jacobweber commented 6 years ago

You shouldn't need to add the platform/plugin again, since they're already in the config.xml. Try just unzipping it and running cordova prepare, or cordova prepare ios; that will restore the node modules and platforms/plugins.

I'm using cordova-cli 8.0.0. Just tried it with Node 9.4.0 and got the same result, but with more detail:

(node:75203) UnhandledPromiseRejectionWarning: TypeError: doc.find is not a function
    at Object.resolveParent (/path/to/.nvm/versions/node/v9.4.0/lib/node_modules/cordova/node_modules/cordova-common/src/util/xml-helpers.js:207:26)
    at /path/to/.nvm/versions/node/v9.4.0/lib/node_modules/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigChanges.js:345:53
    at Array.forEach (<anonymous>)
    at is_conflicting (/path/to/.nvm/versions/node/v9.4.0/lib/node_modules/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigChanges.js:337:17)
    at PlatformMunger.add_config_changes (/path/to/.nvm/versions/node/v9.4.0/lib/node_modules/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigChanges.js:188:33)
    at /path/to/.nvm/versions/node/v9.4.0/lib/node_modules/cordova/node_modules/cordova-lib/src/cordova/prepare.js:130:32
    at _fulfilled (/path/to/CordovaTest/platforms/ios/cordova/node_modules/q/q.js:854:54)
    at self.promiseDispatch.done (/path/to/CordovaTest/platforms/ios/cordova/node_modules/q/q.js:883:30)
    at Promise.promise.promiseDispatch (/path/to/CordovaTest/platforms/ios/cordova/node_modules/q/q.js:816:13)
    at /path/to/CordovaTest/platforms/ios/cordova/node_modules/q/q.js:624:44
(node:75203) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:75203) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
peterpeterparker commented 6 years ago

@jacobweber oki doki, I did and could confirm that I faced the same error as the last one you displayed

don't know why I don't face it with my main project. maybe the use another plugin, like camera, fix it. no idea

jacobweber commented 6 years ago

@EddyVerbruggen Any chance we can get a fix for this? You can reproduce it like this, using cordova-cli 8.0.0 and Node 8.9.4:

cordova create CordovaTest com.sample.cordovatest CordovaTest
cordova create CordovaTest com.jacobweber.cordovatest CordovaTest
cordova platform add ios@4.5.4
cordova plugin add cordova-plugin-customurlscheme --variable URL_SCHEME=x-com-test
rm -rf platforms/ node_modules/ plugins/ package-lock.json

then edit config.xml and add:

    <edit-config file="*-Info.plist" mode="merge" target="NSCameraUsageDescription">
        <string>foo</string>
    </edit-config>

then run cordova prepare --verbose, and you'll get:

(node:9484) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: doc.find is not a function
(node:9484) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
EddyVerbruggen commented 6 years ago

@jacobweber Happy to merge a PR.

jacobweber commented 6 years ago

After some research, I'm thinking this may be a cordova-cli bug when you have edit-config targeting Info.plist in both a plugin and config.xml, possibly related to https://issues.apache.org/jira/browse/CB-13564.

gbarrionuevo commented 6 years ago

any update?

codyrushing commented 5 years ago

Can we get a fork @jacobweber ?

I have other plugins that are requiring me to use cordova 8, and this one that is requiring that I stay at 7. Isn't phonegap development fun 🎉 😡

jacobweber commented 5 years ago

@codyrushing I haven't tried to fix the underlying Cordova bug myself, and Cordova development these days seems....under-resourced.

But I managed to work around it by avoiding <edit-config> in my project's config.xml. Instead I'm using <custom-config-file> from the cordova-custom-config plugin. Not ideal, but it works.

codyrushing commented 5 years ago

@jacobweber hmm. Even if I wanted to use cordova-custom-config as a workaround, it says that it won't work with Phonegap Build, which unfortunately is a requirement for my project