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

[Refactor] Add cross-platform Linking module #5336

Closed
wants to merge 7 commits into from
Closed

[Refactor] Add cross-platform Linking module #5336

wants to merge 7 commits into from

Conversation

satya164
Copy link
Contributor

A promise based API for handling Link for Android and iOS. Refer #4971

The iOS part doesn't handle errors. Will need someone with iOS knowledge to do that.

cc @skevy @ide @brentvatne @mkonicek @vjeux @nicklockwood

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @mkonicek and @satya164 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 Jan 15, 2016
@satya164
Copy link
Contributor Author

We also need to decide what to do with IntentAndroid and LinkingIOS.

  1. Show warnings stating that they are deprecated and use 'Linking', while they continue to work.
  2. Show errors stating that they are deprecated and use 'Linking', making them noop.

It'd also be nice to provide a codemod to automate the transition. Will see if I can work on it.

@satya164 satya164 changed the title Add cross-platform linking component [WIP] Add cross-platform linking component Jan 15, 2016
@satya164 satya164 changed the title [WIP] Add cross-platform linking component [WIP] Add cross-platform Linking component Jan 15, 2016
@satya164 satya164 changed the title [WIP] Add cross-platform Linking component [WIP] Add cross-platform Linking module Jan 15, 2016
@satya164 satya164 mentioned this pull request Jan 15, 2016
@nicklockwood
Copy link
Contributor

@satya164

We also need to decide what to do with IntentAndroid and LinkingIOS.

Option 1 is my preferred approach. Then in the next release we can maybe move to option 2, and then eventually remove them altogether.

@satya164
Copy link
Contributor Author

@nicklockwood Awesome. I'll add the warnings.

@facebook-github-bot
Copy link
Contributor

@satya164 updated the pull request.

@satya164
Copy link
Contributor Author

@skevy @chirag04 Wanna help with the iOS implementation?

@facebook-github-bot
Copy link
Contributor

@satya164 updated the pull request.

@satya164
Copy link
Contributor Author

cc @dmmiller @nicklockwood

@satya164 satya164 changed the title [WIP] Add cross-platform Linking module [Refactor] Add cross-platform Linking module Jan 23, 2016
*/
static getInitialURL(callback: Function) {
console.warn('"IntentAndroid.getInitialURL" is deprecated. Use the promise based "Linking.getInitialURL" instead.');
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@mkonicek
Copy link
Contributor

Awesome! Thanks for working on this!

@@ -85,9 +86,11 @@ public void openURL(String url) {
intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
getReactApplicationContext().startActivity(intent);
}

promise.resolve(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused on what should I resolve with here.

@facebook-github-bot
Copy link
Contributor

@satya164 updated the pull request.

*/
static addEventListener(type: string, handler: Function) {
if (Platform.OS === 'android') {
console.error('Linking.addEventListener is not supported on Android');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this can be just a warning, and not error?

@christopherdro
Copy link
Contributor

Nice one @satya164!
I'm wondering if we can work on getting https://github.com/facebook/react-native-applinks to work with this new API and have android support?

@mkonicek
Copy link
Contributor

Had a quick look again, once you're happy with it feel free to shipit :)

@satya164
Copy link
Contributor Author

@christopherdro I don't use that lib, but if someone can fix it to work with this change, it'll surely be great.

@mkonicek Only one doubt, what should I resolve with here - https://github.com/facebook/react-native/pull/5336/files#diff-fab623097f5131602a9aa8e4829fb6a9R90

null seems to be good candidate, but then I've no idea how to do the same with iOS - https://github.com/facebook/react-native/pull/5336/files#diff-07733d30787de00582b6e9693d4be698R83

@facebook-github-bot
Copy link
Contributor

@satya164 updated the pull request.

@satya164
Copy link
Contributor Author

Talked to @dmmiller . He's fine with true.

@facebook-github-bot
Copy link
Contributor

@satya164 updated the pull request.

@satya164
Copy link
Contributor Author

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/844257825700338/int_phab to review.

@ghost ghost closed this in e33e6ab Jan 26, 2016
@satya164 satya164 deleted the linking branch January 26, 2016 22:40
doostin pushed a commit to doostin/react-native that referenced this pull request Feb 1, 2016
Summary:
A promise based API for handling Link for Android and iOS. Refer facebook#4971

The iOS part doesn't handle errors. Will need someone with iOS knowledge to do that.

cc skevy ide brentvatne mkonicek vjeux nicklockwood
Closes facebook#5336

Reviewed By: svcscm

Differential Revision: D2866664

Pulled By: androidtrunkagent

fb-gh-sync-id: 67e68a827e6b85886bfa84e79b897f079e78b1b5
This pull request was closed.
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