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

[BUGFIX][IOS][LINKS] Update linking restorationHandler implementation for iOS12 #2216

Merged
merged 6 commits into from
Jun 10, 2019

Conversation

xzilja
Copy link
Contributor

@xzilja xzilja commented Jun 8, 2019

Summary

If project is used within Swift environment, there is an error thrown on iOS 12+ devices. Implementation needs to address this and this PR updates it as per official firebase https://firebase.google.com/docs/dynamic-links/ios/receive documentation for linking.

Fixes dynamic links for Swift based projects

Checklist

  • Supports Android
  • Supports iOS
  • e2e tests added or updated in /tests/e2e/*
  • Updated the documentation in the docs repo
    • LINK TO DOCS PR HERE
  • Flow types updated
  • Typescript types updated

Test Plan

Release Plan

[IOS][ENHANCEMENT] [LINKS] - Updated universal link handling implementation for iOS 12


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@mikehardy
Copy link
Collaborator

@xzilja xzilja added plugin: links Firebase Dynamic Links platform: ios labels Jun 8, 2019
@Salakar
Copy link
Member

Salakar commented Jun 9, 2019

CI back to normal, the failure relating to the detox patch was just a blip.

The current failure on here is because iOS is failing to build due to these changes; see the logs of ci.ios.build - build error from the logs:

image

And more errors above that on the logs

@xzilja
Copy link
Contributor Author

xzilja commented Jun 9, 2019

Had a typo, build passed locally for test. Fingers crossed for CI now.

@mikehardy
Copy link
Collaborator

I verified this vs the docs (thanks for the link @iljadaderko ) and this looks clean. CI went green, I'm going to tag this for 5.4.x and +1 in it

I'll let it sit a couple days in case there any objections, then merge for release early next week I hope

@mikehardy mikehardy added this to the v5.4.x milestone Jun 9, 2019
@mikehardy
Copy link
Collaborator

@Salakar I don't have the Objective-C chops to adapt this v5 patch to your v6 code sadly, as it has skewed a bit, but you need to do the same thing right in this spot https://github.com/invertase/react-native-firebase/blob/master/packages/links/ios/RNFBLinks/RNFBLinksAppDelegateInterceptor.m#L55

@xzilja
Copy link
Contributor Author

xzilja commented Jun 9, 2019

@mikehardy I'll open pr to v6 as well, will try to do it some time tomorrow

@Salakar
Copy link
Member

Salakar commented Jun 9, 2019

This is not required at all in v6, internally we swizzle the app delegate methods directly. So there's no longer any need to add it to the users project app delegate either, package is just install and go.

@codecov
Copy link

codecov bot commented Jun 10, 2019

Codecov Report

❗ No coverage uploaded for pull request base (v5.x.x@3146a2c). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             v5.x.x    #2216   +/-   ##
=========================================
  Coverage          ?   72.74%           
=========================================
  Files             ?       64           
  Lines             ?     1599           
  Branches          ?        0           
=========================================
  Hits              ?     1163           
  Misses            ?      436           
  Partials          ?        0

@mikehardy mikehardy changed the title Update iOS linking restorationHandler implementation [BUGFIX][IOS][LINKS] Update linking restorationHandler implementation for iOS12 Jun 10, 2019
@mikehardy mikehardy merged commit 4c08c6d into v5.x.x Jun 10, 2019
@mikehardy
Copy link
Collaborator

thank you @iljadaderko ! this will be in 5.4.3

@mikehardy mikehardy deleted the fix/linking branch June 13, 2019 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: ios plugin: links Firebase Dynamic Links
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants