Skip to content

[Refactor] Add cross-platform Linking module - #5336

Closed
satya164 wants to merge 7 commits into
react:masterfrom
callstack-internal:linking
Closed

[Refactor] Add cross-platform Linking module#5336
satya164 wants to merge 7 commits into
react:masterfrom
callstack-internal:linking

Conversation

@satya164

Copy link
Copy Markdown
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
Copy Markdown
Contributor

By analyzing the blame information on this pull request, we identified @mkonicek and @satya164 to be potential reviewers.

@facebook-github-botfacebook-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
Copy Markdown
ContributorAuthor

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.

@satya164satya164 changed the title Add cross-platform linking component[WIP] Add cross-platform linking componentJan 15, 2016
@satya164satya164 changed the title [WIP] Add cross-platform linking component[WIP] Add cross-platform Linking componentJan 15, 2016
@satya164satya164 changed the title [WIP] Add cross-platform Linking component[WIP] Add cross-platform Linking moduleJan 15, 2016
@satya164satya164 mentioned this pull request Jan 15, 2016
@nicklockwood

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

@nicklockwood Awesome. I'll add the warnings.

@chirag04chirag04 mentioned this pull request Jan 15, 2016
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@satya164 updated the pull request.

@satya164

Copy link
Copy Markdown
ContributorAuthor

@skevy@chirag04 Wanna help with the iOS implementation?

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@satya164 updated the pull request.

@satya164

Copy link
Copy Markdown
ContributorAuthor

cc @dmmiller@nicklockwood

@satya164satya164 changed the title [WIP] Add cross-platform Linking module[Refactor] Add cross-platform Linking moduleJan 23, 2016

Copy link
Copy Markdown
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
Copy Markdown
Contributor

Awesome! Thanks for working on this!

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

@satya164 updated the pull request.

Comment threadLibraries/Linking/Linking.js Outdated

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
Contributor

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

@satya164

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

@satya164 updated the pull request.

@satya164

Copy link
Copy Markdown
ContributorAuthor

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

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@satya164 updated the pull request.

@satya164

Copy link
Copy Markdown
ContributorAuthor

@facebook-github-bot shipit

@facebook-github-bot

Copy link
Copy Markdown
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.

@ghostghost closed this in e33e6abJan 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 react#4971
The iOS part doesn't handle errors. Will need someone with iOS knowledge to do that.
cc skevy ide brentvatne mkonicek vjeux nicklockwood
Closesreact#5336
Reviewed By: svcscm
Differential Revision: D2866664
Pulled By: androidtrunkagent
fb-gh-sync-id: 67e68a827e6b85886bfa84e79b897f079e78b1b5
This pull request was closed.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis 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

@satya164@facebook-github-bot@nicklockwood@mkonicek@christopherdro