refactor: universal link handling in the sample app #181
Conversation
|
Pull request title looks good 👍! If this pull request gets merged, it will not cause a new release of the software. Example: If this project's latest release version is All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time. This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...This project uses a special format for pull requests titles. Don't worry, it's easy! This pull request title should be in this format: If your pull request introduces breaking changes to the code, use this format: where
Examples:Need more examples? Want to learn more about this format? Check out the official docs. Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests. |
| NSURL *url = userActivity.webpageURL; | ||
| if (!url) { | ||
| return NO; | ||
| } | ||
| NSURL *universalLinkUrl = [NSURL URLWithString:@"http://www.amiapp-reactnative-apns.com"]; | ||
|
|
||
| // return true from this function if your app handled the deep link. | ||
| // return false from this function if your app did not handle the deep link and you want sdk to open the URL in a browser. | ||
| if (([url.scheme isEqualToString:@"http"] || [url.scheme isEqualToString:@"https"]) && | ||
| [url.host isEqualToString:universalLinkUrl.host]) { | ||
| return [RCTLinkingManager application:application | ||
| continueUserActivity:userActivity | ||
| restorationHandler:restorationHandler]; | ||
| } else { | ||
| return NO; | ||
| } | ||
|
|
There was a problem hiding this comment.
From my understanding of RN deep linking, the steps to take is:
- Call functions on RCTLinkingManager object in your AppDelegate. Like the code before this PR was doing.
RCTLinkingManagercaptures the URL and stores it for you to use later in javascript code. - Then, you can use the RN navigation module in your javascript code to handle a deep link.
This PR looks like it's suggesting that universal link deep links are handled manually in native code and skip RN deep link handling.
Is this PR to demonstrate a workaround that we need to do? Is there a way that customers can handle universal links in their app using javascript with our SDK? If both of those are true, I would suggest we create a feature ticket to improve upon a workaround and then I'm OK with approving this.
There was a problem hiding this comment.
Your understanding of what it does is correct.
The use-case is if the app doesn't handle universal links open it in the browser. Currently, if the app doesn't handle the universal link it doesn't do anything.
It can be solved via JS code or by letting our SDK do it for you. If we do it via JS code it makes it look like it should be how its done? so handling it this way to show users SDK can do deep-link handling if their app doesn't do it.
I am open to ideas here. But this was made out of LJs ticket where she though SDK is broken because its not doing the redirection.
There was a problem hiding this comment.
Just wanted to highlight, if I remember correctly, we currently open universal links automatically to browsers if they are not supported by app in Android. So if we are considering any solution that recommends JS code to handle this, there will be inconsistency and might create confusions for some customers.
There was a problem hiding this comment.
Ok, from reading the responses, this sounds like we are trying to make this fix:
Expected behavior: app does not handle a universal link -> SDK opens the URL in the system browser.
Actual behavior: app does not handle a universal link -> nothing happens. Browser does not open the URL.
Because it's preferred that our RN SDK has an API that uses JS code more then native code, I would consider this proposed change to be a workaround. Therefore, I am OK with merging this PR and us making a ticket for improving this in the future.
We do have a feature request for improving RN deep link handling already. I'll add a note to that ticket. If someone believes this is a different problem, please create a separate ticket.
No description provided.