Add deep linking support to IntentAndroid #4320

satya164 · 2015-11-24T12:03:59Z

Add a method to handle URLs registered to the app,

IntentAndroid.getInitialURL(url => {
    if (url) {
        // do stuff
    }
});

Refer - http://developer.android.com/training/app-indexing/deep-linking.html#adding-filters

The API cannot be same as the iOS API (i.e. as a constant), as the activity is not availble at the time of module initialization. Moreover, multiple activties can share the same bridge instance, and the activity itself is not a constant. Hence the initialURL can change.

facebook-github-bot · 2015-11-24T12:05:24Z

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

baoti · 2015-11-25T14:19:39Z

ReactAndroid/src/main/java/com/facebook/react/modules/intent/IntentModule.java

+  public Map<String, Object> getConstants() {
+    final Map<String, Object> constants = new HashMap<>();
+
+    Intent intent = ((Activity) mActivityContext).getIntent();


Why not mActivity.getIntent() ?

There's no mActivity here! O.o

Just change all (activity's) Context to Activity class

Because it should be a Activity instance.

mkonicek · 2015-11-25T19:51:13Z

Nice! Thanks for working on this!

satya164 · 2015-11-26T00:37:39Z

@mkonicek Thanks. Waiting for the Dialog commit before I change anything here :D

facebook-github-bot · 2015-12-03T12:59:16Z

@satya164 updated the pull request.

facebook-github-bot · 2015-12-03T13:07:15Z

@satya164 updated the pull request.

facebook-github-bot · 2015-12-03T13:08:12Z

@satya164 updated the pull request.

facebook-github-bot · 2015-12-03T13:23:17Z

@satya164 updated the pull request.

facebook-github-bot · 2015-12-04T03:06:14Z

@satya164 updated the pull request.

facebook-github-bot · 2015-12-04T10:57:15Z

@satya164 updated the pull request.

satya164 · 2015-12-04T11:19:53Z

@mkonicek I've updated the PR using getCurrentActivity() implemented in c06efc0.

However, the current activity is only set on onResume, so it's not available while calling getConstants method, to have a consistent API between iOS and Android we need that. I've added a method to set the current activity and added the code to the generator to set the activity in onCreate. It's backward compatible, but if someone wants the deep linking feature, and has created the project with the old template, he'll need to manually add the code required.

I'm not sure if it's the best approach though. Please have a look and lemme know.

cc @foghina

mkonicek · 2015-12-09T21:55:36Z

Just pinged @foghina, thanks for the patience. Many PRs in the queue and branch cut..

mkonicek · 2015-12-09T21:57:52Z

ReactAndroid/src/main/java/com/facebook/react/modules/intent/IntentModule.java

+
+    constants.put("initialURL", initialURL);
+
+    return constants;


This seems reasonable - the process could be started by an intent.

foghina · 2015-12-10T11:09:52Z

I don't like exposing setActivity on ReactInstanceManager.Builder. This sets the expectation that the activity will never change, but in fact, it can change. This is why it's called getCurrentActivity() and not getActivity(). Also, exposing the intent URL as a constant furthers this assumption and will break when you change activities without recreating the ReactContext.

Since the JS API is only similar but not identical and since the JS API is a function call anyway (IntentAndroid.popInitialURL()), you can easily rewrite it to call into a native method, instead of reading a native constant. Then all the changes in ReactInstanceManager and co. become unnecessary.

Exposing constants has bitten us in the past (see screen size), they should only be used for things that are really constant.

satya164 · 2015-12-10T14:18:16Z

@foghina @mkonicek Should I change this to a function getInitialURL which takes a callback then?

Note that this then diverges from the iOS API. Any thoughts on what should we do in the cross platform module?

facebook-github-bot · 2015-12-13T11:25:15Z

@satya164 updated the pull request.

facebook-github-bot · 2015-12-13T11:32:04Z

@satya164 updated the pull request.

facebook-github-bot · 2015-12-13T11:37:19Z

@satya164 updated the pull request.

facebook-github-bot · 2015-12-13T11:38:22Z

@satya164 updated the pull request.

facebook-github-bot · 2015-12-13T11:39:16Z

@satya164 updated the pull request.

satya164 · 2015-12-13T11:45:13Z

@foghina @mkonicek Changed to use a callback instead of a constant to get initial URl.

facebook-github-bot · 2015-12-15T05:58:11Z

@satya164 updated the pull request.

foghina · 2015-12-15T12:20:28Z

ReactAndroid/src/main/java/com/facebook/react/modules/intent/IntentModule.java

+
+      callback.invoke(initialURL);
+    } catch (Exception e) {
+      throw new JSApplicationIllegalArgumentException(


Can you add an error callback (all @ReactMethods have two callbacks) and call it here instead of throwing an exception? You should probably still log the full exception details, though, to make it easier to debug problems.

@foghina Was wondering the same thing. I kept it consistent with the canOpenURL method which is in the same file. It's really confusing.

Oh, darn. Then we can probably leave it as it is for the purposes of this PR but we should refactor this module.

@foghina Yes, we should. I'll send another PR with the changes then, after this is merged.

foghina · 2015-12-15T13:19:14Z

@facebook-github-bot shipit

facebook-github-bot · 2015-12-15T13:19:57Z

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

foghina · 2015-12-15T13:34:53Z

@satya164 heads-up: patch failed internally because we don't mirror website/, so I had to remove the changes to that folder. Once this lands and is synced back you'll have to reapply your changes to website/. Sorry :(

satya164 · 2015-12-15T13:36:43Z

@foghina No probs. I'll send another PR with the website changes :)

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 Nov 24, 2015

baoti reviewed Nov 25, 2015
View reviewed changes

Add deep linking support to IntentAndroid

ea81f32

Add docs on handling deep links

50a69e0

satya164 mentioned this pull request Dec 4, 2015

Universal Linking component #4546

Closed

Handle if currentActivity is null

00b2f6b

mkonicek reviewed Dec 9, 2015
View reviewed changes

foghina added GH Review: needs-revision and removed GH Review: review-needed labels Dec 10, 2015

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Dec 13, 2015

Use a function instead of a constant to get initial URL

57a936b

Add IntentAndroid to the API docs

10a8084

Fix indentation

b8305b0

satya164 added Size/M labels Dec 13, 2015

Merge branch 'master' into intent

43f2555

foghina reviewed Dec 15, 2015
View reviewed changes

foghina added GH Review: accepted and removed GH Review: review-needed labels Dec 15, 2015

satya164 closed this in eb188c8 Dec 15, 2015

satya164 deleted the intent branch December 30, 2015 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deep linking support to IntentAndroid #4320

Add deep linking support to IntentAndroid #4320

satya164 commented Nov 24, 2015

facebook-github-bot commented Nov 24, 2015

baoti Nov 25, 2015

satya164 Nov 25, 2015

baoti Nov 25, 2015

baoti Nov 25, 2015

mkonicek commented Nov 25, 2015

satya164 commented Nov 26, 2015

facebook-github-bot commented Dec 3, 2015

facebook-github-bot commented Dec 3, 2015

facebook-github-bot commented Dec 3, 2015

facebook-github-bot commented Dec 3, 2015

facebook-github-bot commented Dec 4, 2015

facebook-github-bot commented Dec 4, 2015

satya164 commented Dec 4, 2015

mkonicek commented Dec 9, 2015

mkonicek Dec 9, 2015

foghina commented Dec 10, 2015

satya164 commented Dec 10, 2015

facebook-github-bot commented Dec 13, 2015

facebook-github-bot commented Dec 13, 2015

facebook-github-bot commented Dec 13, 2015

facebook-github-bot commented Dec 13, 2015

facebook-github-bot commented Dec 13, 2015

satya164 commented Dec 13, 2015

facebook-github-bot commented Dec 15, 2015

foghina Dec 15, 2015

satya164 Dec 15, 2015

foghina Dec 15, 2015

satya164 Dec 15, 2015

foghina commented Dec 15, 2015

facebook-github-bot commented Dec 15, 2015

foghina commented Dec 15, 2015

satya164 commented Dec 15, 2015

Add deep linking support to IntentAndroid #4320

Add deep linking support to IntentAndroid #4320

Conversation

satya164 commented Nov 24, 2015

facebook-github-bot commented Nov 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek commented Nov 25, 2015

satya164 commented Nov 26, 2015

facebook-github-bot commented Dec 3, 2015

facebook-github-bot commented Dec 3, 2015

facebook-github-bot commented Dec 3, 2015

facebook-github-bot commented Dec 3, 2015

facebook-github-bot commented Dec 4, 2015

facebook-github-bot commented Dec 4, 2015

satya164 commented Dec 4, 2015

mkonicek commented Dec 9, 2015

Choose a reason for hiding this comment

foghina commented Dec 10, 2015

satya164 commented Dec 10, 2015

facebook-github-bot commented Dec 13, 2015

facebook-github-bot commented Dec 13, 2015

facebook-github-bot commented Dec 13, 2015

facebook-github-bot commented Dec 13, 2015

facebook-github-bot commented Dec 13, 2015

satya164 commented Dec 13, 2015

facebook-github-bot commented Dec 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foghina commented Dec 15, 2015

facebook-github-bot commented Dec 15, 2015

foghina commented Dec 15, 2015

satya164 commented Dec 15, 2015