-
Notifications
You must be signed in to change notification settings - Fork 92
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
Adds WatchKit support via CocoaPods #465
Adds WatchKit support via CocoaPods #465
Conversation
Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://github.com/snowplow/snowplow/wiki/CLA to learn more and sign. Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks. |
I signed it! |
Thanks for your contribute @leoAsana . We are going to release soon the version 1.1.4 (early next week). Please, could you rebase your branch onto master as soon as we proceed with the release of 1.1.4 ? |
I've rebased I know this is a rather large PR, but most changes are just in the example projects. |
Thanks @leoAsana. That's very helpful, I'll review the changes in the next days. |
@AlexBenny Thanks! I'm already using by fork in our watch project and I havn't seen any problems yet. I noticed that the platform is |
I noticed that apps using this library could not launch on older watchOS version. This adds a demo project to reproduce this,
This fixes the crash mentioned in the prev. commit. It removes the frameworks for watchOS from the Podspec.
@leoAsana Sorry for the delays, and happy to see that your solution is working without issues. Unfortunately we’re working on some of the core features, that slows down the integration of GitHub contributes, but once we have the high priority work done we will review your PR.
We haven't defined anything for wearables at the moment. I think it would make sense using FYI: we just released the version 1.1.5 where we fixed an issue introduced on 1.1.4 about IDFA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the important contribute on making our tracker suitable for the Apple Watch!
I've reviewed and tested the code on the device. It works very well.
I've looked briefly to the demo apps and it seems everything good.
We can't consider the Watch tracker ready to production, we need to discuss it internally and eventually plan further improvements, but I would be happy to merge it in the next release.
I've just added few comments to address before the merge.
I'd ask you to add also this small details:
- SPUtilities.m > getOSType - Can we return @"watchos" for the Watch target?
- Snowplow.m > Could you add "watchos-1.1.5" to kSPVersion?
} else { | ||
NSLog(@"SPLog: Error: %@", connectionError); | ||
[results addObject:[[SPRequestResponse alloc] initWithBool:false withIndex:indexArray]]; | ||
[_dataOperationQueue addOperationWithBlock:^{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
We should refactor all that, soon or later, but meanwhile this is a nice fix for the deprecated method!
Examples/SnowplowSwiftCocoapodsDemo/SnowplowSwiftWatch/Base.lproj/Interface.storyboard
Outdated
Show resolved
Hide resolved
Thanks for the review. I applied all the request changes. |
Thanks @leoAsana for your great work! |
* Adds WatchKit support via CocoaPods * Fixes missing file for watchOS * Change watchOS product name * Adds modual map for watchos * Adds carthage watch example * Adds cocoapods watch example * Update CocoaPods demo to include older watchOS version I noticed that apps using this library could not launch on older watchOS version. This adds a demo project to reproduce this, * Fix crash on < watchOS 6 This fixes the crash mentioned in the prev. commit. It removes the frameworks for watchOS from the Podspec. * Changed button name
* Adds WatchKit support via CocoaPods * Fixes missing file for watchOS * Change watchOS product name * Adds modual map for watchos * Adds carthage watch example * Adds cocoapods watch example * Update CocoaPods demo to include older watchOS version I noticed that apps using this library could not launch on older watchOS version. This adds a demo project to reproduce this, * Fix crash on < watchOS 6 This fixes the crash mentioned in the prev. commit. It removes the frameworks for watchOS from the Podspec. * Changed button name
PR #478 * Adds WatchKit support via CocoaPods (#465) * Fixes missing file for watchOS * Change watchOS product name * Adds modual map for watchos * Adds carthage watch example * Adds cocoapods watch example * Update CocoaPods demo to include older watchOS version I noticed that apps using this library could not launch on older watchOS version. This adds a demo project to reproduce this, * Fix crash on < watchOS 6 This fixes the crash mentioned in the prev. commit. It removes the frameworks for watchOS from the Podspec. * Changed button name * Update podspec for watchOS target Updated podspec excluding the new Reachability feature added when we got rid of the third part dependency Reachability.swift * Update travis scripts * Pod update Co-authored-by: leoAsana <56038088+leoAsana@users.noreply.github.com>
As mentioned in my issue ##464 was was looking to use the tracker from the Apple Watch directly.
I went ahead and fixed the issues that prevented the project to build on watchOS and added watchOS to the podspec.
The changes a just some more
#if
s inSPUtilities.m
and I had to replace the use ofNSURLConnection
inSPEmitter.m
withNSURLSession
. I used adispatch_semaphore
to make it synchronous as mentioned here.I've also added an watch app to the Swift Demo and it worked in my tests.