Add deep linking support to IntentAndroid

[satya164]

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]

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

[baoti]

Why not mActivity.getIntent() ?

[satya164]

There's no mActivity here! O.o

[baoti]

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

[baoti]

Because it should be a Activity instance.

[mkonicek]

Nice! Thanks for working on this!

[satya164]

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

[facebook-github-bot]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[satya164]

@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]

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

[mkonicek]

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

[foghina]

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]

@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]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[facebook-github-bot]

@satya164 updated the pull request.

[satya164]

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

[facebook-github-bot]

@satya164 updated the pull request.

[foghina]

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.

[satya164]

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

[foghina]

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

[satya164]

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

[foghina]

@facebook-github-bot shipit

[facebook-github-bot]

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]

@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]

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