Skip to content
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

Fixing unwanted pending dynamic links checks on subsequent app restarts #5665

Merged
merged 5 commits into from
May 29, 2020

Conversation

eldhosembabu
Copy link
Contributor

@eldhosembabu eldhosembabu commented May 21, 2020

Fix for b/154589338

Most of the times, when the app is cold started, we are making api calls to:

https://firebasedynamiclinks-ipv4.googleapis.com/v1
https://firebasedynamiclinks-ipv6.googleapis.com/v1

Until we get a successful response. But sometimes, if the device doesn’t have support for v6 endpoint and if the v4 endpoint doesn’t retrieve a valid dynamic link, it will return error. This mostly happens when user directly goes to App Store and installs an app which has fdl sdk integrated.

For dynamic links, we want the app to retrieve the pending dynamic link on the very first start of the app after install so that 3p developers can deeplink to a particular page in the app. If the pending links retrieval failed during the first start, there is no point in retrying the retrieval during next restart since it could result in an un expected deep link for the user when they opens up the app for subsequent restarts (if a valid dynamic link is returned).

Most of the times, when the app is cold started, we are making api calls to:

https://firebasedynamiclinks-ipv4.googleapis.com/v1
https://firebasedynamiclinks-ipv6.googleapis.com/v1

Until we get a successful response. But sometimes, if the device doesn’t have support for v6 endpoint and if the v4 endpoint doesn’t retrieve a valid dynamic link, it will return error. This mostly happens when user directly goes to App Store and installs an app which has fdl sdk integrated.

For dynamic links, we want the app to retrieve the pending dynamic link on the very first start of the app after install so that 3p developers can deeplink to a particular page in the app. If the pending links retrieval failed during the first start, there is no point in retrying the retrieval during next restart since it could result in an un expected  deep link for the user when they opens up the app for subsequent restarts (if a valid dynamic link is returned).
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @eldhosembabu!

Please investigate adding a unit test.

Please fix the GHA issues by running clang-format. See https://github.com/firebase/firebase-ios-sdk#code-formatting

While ok to put internal bug numbers in PRs, it should not be in the title. Instead a short description of the bug fix should be there.

Also the PR description should include a note about the GitHub issue being fixed. With "Fix #{bug number}", GitHub will automatically close the related bug when the PR merges.

FirebaseDynamicLinks/Sources/FIRDynamicLinks.m Outdated Show resolved Hide resolved
@eldhosembabu eldhosembabu changed the title Fix for b/154589338 May 27, 2020
@eldhosembabu
Copy link
Contributor Author

Thanks for the PR @eldhosembabu!

Please investigate adding a unit test.
Added a unit test
Please fix the GHA issues by running clang-format. See https://github.com/firebase/firebase-ios-sdk#code-formatting
Done
While ok to put internal bug numbers in PRs, it should not be in the title. Instead a short description of the bug fix should be there.
Done, please let me know the description looks good.
Also the PR description should include a note about the GitHub issue being fixed. With "Fix #{bug number}", GitHub will automatically close the related bug when the PR merges.
The fix was based on an internal bug, didn't find a github issues to mention here.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the followup!

I thought this was also a fix for #5032?

The description looks good. Please also add it to https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseDynamicLinks/CHANGELOG.md. The version can be "Unreleased" for now.

Plus three more spelling nits

FirebaseDynamicLinks/Sources/FIRDynamicLinks.m Outdated Show resolved Hide resolved
FirebaseDynamicLinks/Sources/FIRDynamicLinks.m Outdated Show resolved Hide resolved
@eldhosembabu
Copy link
Contributor Author

Thanks for the followup!

I thought this was also a fix for #5032?
Its not actually, for fixing #5032 we need to optimize the calls i guess for that. Currently we are doing ipv4 and ipv6 calls parallel and consolidate the results which needs to be serialized. In this current fix, I'm just making sure we don't repeat checkforpending calls when there is an error in the previous one.
The description looks good. Please also add it to https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseDynamicLinks/CHANGELOG.md. The version can be "Unreleased" for now.

Plus three more spelling nits
Done

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.