search

apache / cordova-discuss

Discussions on features and the future
19 stars 28 forks source link

CB-13145: proposal for Play Services Version #74

Open audreyso opened 7 years ago

audreyso commented 7 years ago

Proposal for Play Services

goya commented 7 years ago

could we allow it inside a platform tag as well for extra flexibility?

plugin variable || platform variable || global variable

stevengill commented 7 years ago

@goya that sounds reasonable. Lets say yes for now as we look at the implementation details.

janpio commented 7 years ago

Anything that gets rid of these gradle error messages has my full support. Proposal looks good.

filmaj commented 7 years ago

I like @goya's suggestion. Also, why leverage <preference> in config.xml but <variable> in plugin.xml? Could we standardize on <variable> across the board?

stevengill commented 7 years ago

@filmaj sorry for the confusion. The first solution was to use <preference> but this document is proposing not to do that and instead to just use <variable>

macdonst commented 7 years ago

So just a heads up. That this may end up being more work than initially thought. Right now if we try to work around this by doing:

<preference name="FCM_VERSION" default="11.0.1"/>

and then trying to use it in the framework tag like so:

<framework src="com.google.firebase:firebase-messaging:$FCM_VERSION"/>

results in a build error:

* What went wrong:
A problem occurred evaluating root project 'android'.
> Could not get unknown property 'FCM_VERSION' for object of type org.gradle.api.internal.artifacts.dsl.dependencies.DefaultDependencyHandler.

Because the $FCM_VERSION variable does not get replaced in the framework tag.

stevengill commented 7 years ago

Okay update time!

So @audreyso and I have done some deep investigations into this work.

Firstly, I think our proposal here is too ambitious and overkill for now.

Intro to how plugin variables work now:

Hopefully that explains our current plugin variable situation.

Now, back to the problem.

We need a way to define a variable, and have the <framework> tag consume it. Something like:

<preference name='varName' default='varValue'>

<framework src='someString:$varName'>

This would replace $varName with varValue.

I've sent a PR to cordova-common that adds support for the above solution. https://github.com/apache/cordova-common/pull/4.

When the frameworks are being grabbed from plugin.xml, it will find and replace $varName with varValue. For Android, this then gets written out to the apps project.properties & build.gradle.

One caveat about this solution:

Both the push plugin and the google analytics plugin will have to add a preference for the playServices version. They can manually be kept in sync for now by plugin authors.

For the future, it would be nice if we added the ability to check the variable tags in config.xml with the ability to override the default value when frameworks are being handled. That would require the preferences to have the same name in both plugins.

macdonst commented 7 years ago

@stevengill that's some pretty awesome progress today. However, I think we should support using the variable tag to override the default preference in the plugin.xml file. This way if the preference in the push plugin.xml is to use 11.0.1 and in the google analytics plugin.xml it is 9.8.0 the user would be able to specify something like

    <plugin name="cordova-plugin-google-analytics" spec="^1.4.3">
        <variable name="PLAY_SERVICES_VERSION" value="11.0.1" />
    </plugin>

That would end up giving us the most flexibility as plugin authors would define their own variable/preference name i.e. there is no need to get agreement with other plugin authors on those names. Also, users could override the default version number without having to send a PR to the plugin authors.

goya commented 7 years ago

plugin variable || platform variable || global variable

not a preference element. the preference namespace is too unimaginative to NOT conflict with a random variable some plugin author chooses.

preference element is for core or platform only. IMHO.

stevengill commented 7 years ago

@macdonst I agree. I've now updated the PR to work with variable tags from config.xml. https://github.com/apache/cordova-common/pull/4

I have only implemented it for the framework add usecase (both restore + cli passed in variable). For remove, it still uses getPreferences (aka default value in preference element in plugin.xml). I will have to add cli_variables to the uninstallOptions variable in cordova-lib.

To test this: 1) pull the PR above into your local cordova-common 2) npm link cordova-common to cordova-lib 3) go to local cordova-android repo and open package.json. Update cordova-common dependency to the path to your local cordova-common (npm link won't work)

ex: "cordova-common": "../cordova-common",

You can now run the following commands (on modified push plugin.xml from https://github.com/apache/cordova-discuss/pull/74#issuecomment-322316133)

cordova plugin add ../../../phonegap/phonegap-plugin-push/  --variable FCM_VERSION='10.0.0

or to restore

Add to config.xml:
<plugin name="phonegap-plugin-push" spec="../../../phonegap/phonegap-plugin-push">
        <variable name="FCM_VERSION" value="9.0.0" />
</plugin>

Run:
cordova prepare

Whats left? 1) get remove to use passed in variables or config.xml variables 2) write tests

stevengill commented 7 years ago

@goya I agree that preference element seems wrong to use. But they are being used already for plugin variables (See https://github.com/apache/cordova-discuss/pull/74#issuecomment-323232349). They are tied toconfig-file and edit-config elements.

Are you suggesting we stop using preference even for edit-config and config-file and switch over to using a variable element instead? Or just not use preference for framework variable replacing.

One safety we do have is that the regex looks for $varName to replace instead of just varName.

The way it is setup currently, I don't think we would have clashes from different plugins using the same variable. In config.xml, the variable tag is still currently tied to a specific plugin. When we get the preferences from a plugin's plugin.xml, we can only use the variable tags associated with it. If we introduce global or platform variable tags in config.xml, I could see clashes arising if two plugins used same variable/preference name. I don't think we need to do global/platform variable tags anymore.

We can just expect the user to manually change versions if they need to.

In config.xml

<plugin name="phonegap-plugin-push" spec="../../../phonegap/phonegap-plugin-push">
        <variable name="FCM_VERSION" value="9.0.0" />
</plugin>
<plugin name="cordova-plugin-googleanalytics" spec="../../../phonegap/somePath">
        <variable name="FCM2_VERSION" value="11.0.0" />
</plugin>

In the above example, the user would have to manually make the versions (value) the same and restore. It doesn't matter if the variable name they use is the same or not between the two plugins.

audreyso commented 7 years ago

Hi all! Here are 2 PRs for cordova-lib and cordova-common https://github.com/apache/cordova-lib/pull/590/files https://github.com/apache/cordova-common/pull/6/files

If you could review or have any comments/advice/questions, that would be great! Thanks so much!

audreyso commented 7 years ago

Here are the main points of the solution we ended up using:

Let us know if you have any questions!