[firebase_dynamic_links] Change architecture to work correctly on async links and launch links

[jherencia]

Description

This pull request changes firebase_dynamic_links architecture to be able to difference between the dynamic link which opened the app and links clicked on app active or in background.

Related Issues

• First deeplink is opened all the time flutter#28062
• Firebase Dynamic Link doesn't clear the Link on Android flutter#24629
• FirebaseDynamicLinks retrieveDynamicLink() only retrieves the links in Firebase, not the link that opened the app flutter#27439
• Firebase Dynamic Links cannot open links if application is already running flutter#21456

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
• All existing and new tests are passing.
• I updated/added relevant documentation (doc comments with ///).
• The analyzer (flutter analyze) does not report any problems on my PR.
• I read and followed the Flutter Style Guide.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I signed the CLA.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
• No, this is not a breaking change.

[collinjackson]

Thanks for the PR! Some minor comments, @bparrishMines might want to take a look as well.

[bparrishMines]

Hi @jherencia,

I'm unable to reproduce these bugs with the latest version of firebase_dynamic_links: 0.4.0+3. I think we just forgot to close all the bugs related to the same issue. What version are you using? I believe these bugs were fixed with #1569

[jherencia]

Hello @bparrishMines,

This PR had two goals:

1. to fix that issue, which I see it is fixed in the latest version
2. to change the architecture of the plugin to be able to subscribe to dynamic links events while the application is active or in background in a consistent way with firebase_messaging. I believe that it is a better approach than needing to subscribe to application lifecycle changes.

I've made another PR to firebase_messaging (#1709) in the same direction.

[jherencia]

Hi @bparrishMines, I assume you didn't have time to see this.

Do you know when you'll be able to check this, I'm going to be off in some days and to be able to answer your questions. I really think this is a really important improvement.

[bparrishMines]

@jherencia Unfortunately, this plugin is still lacking integration testing and we are still working on our side to potentially get expresso to work with flutter testing tools (e.g. flutter driver). Since '1.' has already been solved, I don't believe we should go forward with an architecture change without integration tests to make sure everything will still work.

Also, we typically want the dart api to match the native api. The Android API doesn't use a stream to receive links, so I don't believe an architecture change is necessary if it isn't necessary to solve a bug or crash.

[jherencia]

Thank you @bparrishMines for the response, I'm going to try explain why I think this PR is needed even if we do not have integration tests.

1. First of all, the main reason is that the current implementation / architecture does not match the native API either, my PR goes in the direction of providing a consistent way to retrieve the dynamic link that launched the application.

As easy as:

final PendingDynamicLinkData data = await FirebaseDynamicLinks.instance.retrieveDynamicLink();

which follows how Firebase library works in the native part:

FirebaseDynamicLinks.getInstance()
        .getDynamicLink(registrar.activity().getIntent())
        .addOnSuccessListener(
            registrar.activity(),
            new OnSuccessListener<PendingDynamicLinkData>() {
              @Override
              public void onSuccess(PendingDynamicLinkData pendingDynamicLinkData) {
                 // onSuccess logic
              }
            })
        .addOnFailureListener(
            registrar.activity(),
            new OnFailureListener() {
              @Override
              public void onFailure(@NonNull Exception e) {
                // onFailure logic
              }
            });

but the current architecture requires the Dart application to listen to to AppLifecycleChanges, which adds a lot of complexity and risks of failure in the Dart side. Why? It should not be that hard to retrieve the dynamic link. And it wouldn't be necessary to call retrieveDynamicLink every time the application state changes.

2. My PR proposal clearly separates launchLinks and links open when the application is running, which gives the developer more power and follows how Firebase does this in the native way. You subscribe a listener to a new intent as an event, you do not have to check on lifecycle changes:

@Override
  public boolean onNewIntent(Intent intent) {
    FirebaseDynamicLinks.getInstance()
        .getDynamicLink(intent)
        .addOnSuccessListener(
            registrar.activity(),
            new OnSuccessListener<PendingDynamicLinkData>() {
              @Override
              public void onSuccess(PendingDynamicLinkData pendingDynamicLinkData) {
                // onSucessLogic
              }
            })
        .addOnFailureListener(
            registrar.activity(),
            new OnFailureListener() {
              @Override
              public void onFailure(@NonNull Exception e) {
                // onFailure logic
              }
            });

    return false;
  }

I did this using a Future following the best practices used in other plugins and in this one.

3. In conclusion, we've developed 2 different applications with the architecture and lead us to a lot of complexity which was eased with our change.

So I understand why not having integration tests could be a blocker, but I think asking developers to subscribe to app lifecycle changes and to use an inconsistent API is blocker too, as long as makes their life hard and increases the risk of bugs and edge cases in which the "dynamic link" is not syncronized between the native part and the dart part, which has happened a lot in the past and lead to "not so" elegant solutions like:

bc04a77#diff-3f6c424218a2662c8ddc4b322f30bf66

Please, do not miss understand my comment, I understand the hard work behind this but I'm just trying to point that it could me improved with this PR.

I'm happy to make any changes that you think could be better.

Best regards.

[bparrishMines]

Thanks for taking the time to write this out. I agree with your reasoning and believe this feature should be implemented. However, I think it should be implemented as a convenience method rather than an architecture change.

I could imagine an instance where someone would only want to retrieve a single dynamic link, and not want to listen for callbacks. For example, email link authentication. Not to mention some users may currently be using retrieveDynamicLink with their Lifecycle flow and this would cause a regression for them.

Would it be possible to keep the current architecture and add the listener? I also think an EventChannel would be better suited for the listener. The sensors plugin is a good example of using this.

LMK what your thoughts on this are.

[collinjackson]

Since this plugin is pre-1.0 I am assuming some breaking changes are acceptable.

We may be able to draw some inspiration from the React Native Firebase implementation here. It looks like they have two ways that they allow developers to receive links:

1. getInitialLink() which which looks at launchOptions[UIApplicationLaunchOptionsURLKey] (we don't seem to be doing this yet) as well as continueUserActivity on iOS.
2. onLink which provides notifications of all links received after the callback is registered.

Interestingly errors aren't passed over the bridge. They're simply logged to the console. https://github.com/invertase/react-native-firebase/blob/978b298f94940db7776cd6c4117eb6b618c92824/packages/links/ios/RNFBLinks/RNFBLinksAppDelegateInterceptor.m#L74

In the Dart version, onLink might be more Dart-y to expose as a Stream rather than setting callbacks but I could go either way on that. If we do callbacks onLink as a setter might make sense rather than a configure() method.

EventChannel does seem more appropriate than MethodChannel here but it doesn't affect the developer either way.

FYI @Ehesp

[jherencia]

@collinjackson

Yes, the architecture proposed is based in RNFirebase.

• getInitialLink() is what I called getLauchLink to be in line with firebase_messaging naming
• onLink is configure with two callbacks OK and KO, again to be in line with firebase_messaging which uses this same behaviour

I am happy to do any change you think is needed: change the naming, migrate from MethodChannel to EventChannel, etc.

See https://github.com/flutter/plugins/tree/master/packages/firebase_messaging and my other PR (#1709)

@bparrishMines
The architecture change comes because the dependency with Lifecycle subscription is wrong and leads to mistakes as long as forces developers to handle things in an architecture that is harder than it should be.

RNFirebase does this exact thing:

• offers a way to get the launch / initial link => await getLaunchLink
• offers a way to be notified when a new link is pressed => onLink which I called configure

The PR follows the same architecture.

[bparrishMines]

@jherencia After giving this more thought, I think we should go through with the change and avoid depending on the Lifecycle changes. I left an initial review with a few comments

[jherencia]

@bparrishMines

Great news :).

I pushed two commits:

1. With all your suggestions
2. Renaming configure method to onLink as @collinjackson suggested. It's easy to revert if you both decide between one or the other.

I haven't migrated from MethodChannel to EventChannel as long as it is a big change and probably affects to tests. If you think it should be changed, let me know and I'll work on that.

[bparrishMines]

Hey @jherencia. Sorry for the delay, I've been out sick this week. I left a few more comments. Also, I agree that we can wait for a switch to EventChannel since it won't affect the developer; like @collinjackson mentioned.

[bparrishMines]

LGTM

@collinjackson do you have any more input?

[collinjackson]

I have a mild preference to call the API getInitialLink instead of getLaunchLink for consistency with React Native Firebase and because the internal Firebase API review has approved the getInitialLink method name. If you feel that getLaunchLink is better, let me know and we can re-evaluate the decision, but otherwise I think we should use getInitialLink.

[jherencia]

I've changed the name as suggested.

Thank you both :)

[mohammed932]

thanks for help me to solve my issue