Skip to content

[firebase_dynamic_links] make sure the initial link resolves #2580

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

Closed

Conversation

hffmnn
Copy link
Contributor

@hffmnn hffmnn commented May 18, 2020

Description

We found that sometimes the initial link might not resolve correctly. This is because of a possible race condition:

  1. iOS calls
- (BOOL)application:(UIApplication *)application
    continueUserActivity:(NSUserActivity *)userActivity
      restorationHandler:(void (^)(NSArray *))restorationHandler

and link resolving starts by calling onInitialLink.

  1. handleMethodCall is called with @"FirebaseDynamicLinks#getInitialLink" while the initial link still gets resolved
  2. Because _initialLink is still nil that gets send via the channel.

To resolve this issue, this PR

  1. sets the new BOOL isResolvingInitialLink to YES while resolving.
  2. checks if isResolvingInitialLink is YES when @"FirebaseDynamicLinks#getInitialLink" gets called and waits until it is NO.

Not sure if this solution is too simplistic, but at least it worked for our case.

Related Issues

#1624
#100

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.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • 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].
  • 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.

@degloff
Copy link

degloff commented Oct 9, 2020

@hffmnn thanks for that fix. I tested and seems to work. I tested with a custom deep link and also
with Google Auth email link signin. Both works now when the app is closed. Initial link is resolved properly.

As this is a recurring issue open now for a year can someone with the authority press the merge button? Thanks.

@hffmnn
Copy link
Contributor Author

hffmnn commented Oct 9, 2020

@degloff Good to hear that it works for you.

can someone with the authority press the merge button

Would need quite some conflict resolution and some retesting because master changed quite a bit.

@degloff
Copy link

degloff commented Oct 9, 2020

@hffmnn actually I don't think it is too much of an issue, your changes are just in FLTFirebaseDynamicLinksPlugin.m and that did not change too much. But better if someone looks at it again with good ObjectiveC knowledge.

@koskimas
Copy link

koskimas commented Oct 9, 2020

I think there's a misunderstanding of what getInitialLink is supposed to do. It's not supposed to wait for the initial link as this PR assumes and does. It's supposed to return the link that may or may not have been delivered before the onLink listener was installed.

The actual app start link (initial link) can be delivered to either onLink or get returned by getInitialLink as stated in the documentation. Maybe this fact could be made more clear in the docs though.

@degloff
Copy link

degloff commented Oct 9, 2020

@koskimas Thanks for the clarification. However I exactly tested the ^0.6.0+2 version of the plugin by first registering onLink handlers and then call getInitialLink. However, this does not solve the problem either and the link that opened the closed app, neither was received by onLink nor by getInitialLink. Actually to be precise, in about 1 out of 10 cases it was received by getInitialLink. As we use email link signup and other deep link quite intensively, this is a very bad end user experience.

The downside of @hffmnn implementation is of course that the startup phase of the app shows the white screen for a little longer before the first screen can be rendered.

@hffmnn hffmnn force-pushed the feature/initial-link-resolving-ios branch from 384e058 to 5b6382c Compare March 19, 2021 17:09
@Salakar
Copy link
Member

Salakar commented Apr 21, 2021

Closing in favour of #4354

@Salakar Salakar closed this Apr 21, 2021
@firebase firebase locked and limited conversation to collaborators May 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants