Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

Update plugin to use GCM Cocoapods <framework> reference in plugin.xml #1183

Merged
merged 3 commits into from
Nov 1, 2016
Merged

Update plugin to use GCM Cocoapods <framework> reference in plugin.xml #1183

merged 3 commits into from
Nov 1, 2016

Conversation

shazron
Copy link
Member

@shazron shazron commented Aug 29, 2016

Description

This PR is to remove inclusion of GCM inside this plugin, and replace it Instead with a reference to the Cocoapod itself. A future version of the cordova-cli and the cordova-ios platform will support this new Cocoapod reference.

Motivation and Context

This change facilitates easy updates of GCM in this plugin.

How Has This Been Tested?

Note that pod setup will clone a repo that is 475MB in size, so it might take a while.

Test only with the cli nightlies starting with the nightly released on Monday Sept 5th 8PM PDT or install the cordova-ios platform from the git repo direct

sudo gem install cocoapods
pod setup --verbose
npm install -g cordova@nightly
cordova create test test test
cd test
cordova platform add ios
cordova plugin add https://github.com/shazron/phonegap-plugin-push.git#gcm-cocoapod-support --variable SENDER_ID=xxxTestingxxx
cordova build

After testing, to revert to your original Cordova CLI or the latest:

npm install -g cordova@latest
OR
npm install -g cordova@VERSION

where VERSION is your specific version.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

This is a packaging change, no new tests need to be added.

@jcesarmobile
Copy link
Collaborator

A future version of the cordova-cli and the cordova-ios platform will support this new Cocoapod reference.

Shouldn't the plugin.xml have some property to only install in that future version?

@shazron
Copy link
Member Author

shazron commented Aug 29, 2016

@jcesarmobile yes, see linked reference to the Roadmap issue phonegap/phonegap-roadmap#131 where Steve has already commented on this. I didn't work on that yet since the implementation is incomplete

@shazron
Copy link
Member Author

shazron commented Sep 5, 2016

Updated plugin.xml and package.json for dependencies. However, I have a problem when trying to install this plugin (this PR branch) into a project with cordova-cli 6.3.1:

Plugin doesn't support this project's cordova version. cordova: 6.3.1, failed version requirement: >=6.4.0
Skipping 'phonegap-plugin-push' for ios

It looks like it is honoring the plugin.xml <engines> tag, even though the engines key in package.json should be considered instead. Any insight here @stevengill ?

@shazron
Copy link
Member Author

shazron commented Sep 6, 2016

Found the culprit. This line.

id is https://github.com/shazron/phonegap-plugin-push.git#gcm-cocoapod-support
parsedSpec.version is null
cordova_util.isUrl(id) is true
cordova_util.isDirectory(maybeDir) is false.

Because the second condition is true, that whole statement is true, so it returns the promise, and never gets to getFetchVersion below.

@macdonst
Copy link
Member

macdonst commented Sep 7, 2016

@shazron is this PR good to be merged?

@shazron
Copy link
Member Author

shazron commented Sep 7, 2016

Not yet, I'm waiting on @stevengill to comment on the above since we are not backwards compatible currently.

@stevengill
Copy link

So check it,

if you install the plugin via git or local path, getFetchVersion will not be called. It is only used when installing plugins via npm.

So what this means is if you try to install a plugin that isn't supported by your platform/tools, you get a warning it isn't supported and it won't install. Users have to either use a older version of the plugin or update their projects to use the newer platforms/tools. This is expected behavior. If the user installs the plugin via npm, we will auto fetch the appropriate version based on their installed platforms/tools.

I recommend not merging this change until cordova@6.4.0 and ios@4.3.0 are released. Right now, if people install the push plugin via git and you update master to this change, they will not be able to install and won't be able to update their existing projects since the new versions don't exist yet.

@stevengill
Copy link

If you think most people are installing via npm, then go ahead and merge. Though this change isn't useable until ios@4.3.0 and cordova@6.4.0 are released.

@shazron
Copy link
Member Author

shazron commented Sep 8, 2016

I'll defer to Simon on what he wants to do, to avoid any headaches.

Publish now:

  • npm (cordova-cli < 6.1.0) users will have an upgrade path.
  • npm (cordova-cli >= 6.1.0) users will automatically downgrade to v1.8.1
  • local and git url users will not have an upgrade path (is this a significant no. of users?)
  • we will have to document the last case, to install the plugin at version 1.8.1 explicitly

Publish once the respective cli and platforms are published:

  • npm (cordova-cli < 6.1.0) users will have an upgrade path.
  • npm (cordova-cli >= 6.1.0) users will automatically downgrade to v1.8.1
  • local and git url users will have an upgrade path (upgrade platform, upgrade cli)
  • no docs needed

@macdonst
Copy link
Member

macdonst commented Sep 9, 2016

@shazron why don't we just add information to the docs for local and git url users? It would go in:

https://github.com/phonegap/phonegap-plugin-push/blob/master/docs/INSTALLATION.md

@shazron
Copy link
Member Author

shazron commented Sep 9, 2016

@macdonst sure, let's do that. So are you leaning to "publish now", then?

@macdonst
Copy link
Member

macdonst commented Sep 9, 2016

@shazron yeah, always be publishing. The earlier we get it out the quicker we get feedback and can improve on things.

@shazron
Copy link
Member Author

shazron commented Sep 9, 2016

Ok, I'll update the docs for final review and will let all of you know when it is done.

@shazron
Copy link
Member Author

shazron commented Sep 9, 2016

Ok docs updated for CocoaPods support. Have a look.

@macdonst
Copy link
Member

macdonst commented Sep 9, 2016

@shazron looks great. I'm going to merge on Monday as I don't want to do a release today. I'd prefer not to be fielding support questions all weekend.

@wzoom
Copy link

wzoom commented Sep 19, 2016

Hi, is anything blocking the release still?

@macdonst
Copy link
Member

@wzoom I was traveling all last week so I did not get a chance to do a release of this functionality. It won't really be usable until cordova-ios 4.3.0 is release anyway so there is no big rush.

@jcesarmobile
Copy link
Collaborator

You forgot to change the plugin version on plugin.xml

@shazron
Copy link
Member Author

shazron commented Sep 23, 2016

@jcesarmobile thanks! updated.

Requires cordova-ios 4.3.0 or greater, and cordova-cli 6.4.0 or greater.

Add GoogleCloudMessaging CocoaPod v1.2.0
Add GGLInstanceID CocoaPod v1.2.1
Update package.json for cordovaDependencies cordova and cordova-ios
Update plugin.xml for cordova and cordova-ios dependencies in <engines>
@shazron
Copy link
Member Author

shazron commented Nov 1, 2016

Rebased.

@shazron shazron closed this Nov 1, 2016
@shazron shazron reopened this Nov 1, 2016
@macdonst macdonst merged commit b639d83 into phonegap:master Nov 1, 2016
@shazron shazron deleted the gcm-cocoapod-support branch November 14, 2016 17:10
@lock
Copy link

lock bot commented Jun 3, 2018

This thread has been automatically locked.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants