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

[LinkingIOS] Improve deep linking payload sent to JS on openURL #4955

Closed
wants to merge 1 commit into from

Conversation

arasmussen
Copy link
Contributor

Prior to this commit, just the URL string is sent to JS.

_handleOpenURL(event) {}

where

event = {
  url: 'Scheme://Host/Path?Query=Parameters',
}

This leaves JS to parse the URL when we already have a great parsing
tool (NSURLComponents) in iOS from Apple.

Test Plan - tested the following URLs

  1. url = 'reactnative://foo.bar/baz?a=b&c=d'
event = {
  host: 'foo.bar',
  parameters: {
    a: 'b',
    c: 'd',
  },
  path: '/baz',
  scheme: 'reactnative',
  url: 'reactnative://foo.bar/baz?a=b&c=d',
}
  1. url = 'reactnative://'
event = {
  host: '',
  parameters: {},
  path: '',
  scheme: 'reactnative',
  url: 'reactnative://',
}

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @vjeux, @nicklockwood and @tadeuzagallo to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Dec 24, 2015
@@ -76,6 +76,22 @@ var DEVICE_NOTIF_EVENT = 'openURL';
* }
* ```
*
* The payload sent to JS in `_handleOpenURL` will be in the following format:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_handleOpenURL looks like a private method. So I think we should not specify the name.

@arasmussen
Copy link
Contributor Author

@satya164 _handleOpenURL is just from the example above, it isn't an internal API or anything. Perhaps I should clear up the wording?

@satya164
Copy link
Contributor

@arasmussen Oh, didn't notice that. But yeah, the wording can be better.

@arasmussen
Copy link
Contributor Author

Updated, let me know if that's better.

@facebook-github-bot
Copy link
Contributor

@arasmussen updated the pull request.

@ide
Copy link
Contributor

ide commented Dec 24, 2015

We should do this processing in JS for x-platform support (see https://facebook.github.io/react-native/docs/intentandroid.html and #4546), or even push it into userspace.

@arasmussen
Copy link
Contributor Author

@ide Ah I see that Android deep linking has similarly formatted URLs. Doing the processing in JS makes sense. Looks like URI.js would work great (besides not parsing out the query params for you).

Though I'd argue that since LinkingIOS is iOS only, this change is still net win until we have a more x-platform deep linking module.

@facebook-github-bot
Copy link
Contributor

@arasmussen updated the pull request.

@facebook-github-bot
Copy link
Contributor

@arasmussen updated the pull request.

@arasmussen
Copy link
Contributor Author

Another interesting tidbit is that urijs is about 7KB gzipped. Why required the developer to add an extra library when this comes for free in native? It'd be awesome of RN saved you the trouble.

@nicklockwood
Copy link
Contributor

@arasmussen It turns out that the query param parsing part of NSURLComponents is iOS8+ only, so we can't use it. (I replicated it in only a few lines in RCTUtils though, so maybe it still makes sense to do that part natively if the rest of the code is native?)

NSURLComponents *urlComponents = [NSURLComponents componentsWithURL:URL
resolvingAgainstBaseURL:NO];
NSMutableDictionary<NSString *, NSString *> *parameters = [NSMutableDictionary new];
for (NSURLQueryItem *queryItem in urlComponents.queryItems) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NSURLComponents.queryItems is an iOS8+ api, and we need to support iOS7+ (We got burned by this once already :-))

@satya164
Copy link
Contributor

@arasmussen Any updates on this?

@arasmussen
Copy link
Contributor Author

@nicklockwood Thanks for the feedback, definitely agree.

@satya164 Haven't made time for this recently, probably won't be able to for a while. Doesn't seem far from something shippable though. :/

@facebook-github-bot
Copy link
Contributor

@arasmussen updated the pull request.

@facebook-github-bot
Copy link
Contributor

@arasmussen updated the pull request.

Summary: Adds a "Feedback" section which links to feedback.html (which
has a Product Pains widget so that people can post, search, and vote on
React Native feedback without having to go to productpains.com)

Test Plan: Double checked changes in /support.html and that the widget
loads (via <iframe>) on /feedback.html.
@arasmussen
Copy link
Contributor Author

Don't have time to fix this one up. :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants