feat: create abstract layer for deep_link_client to simplify other implementations

[elianortega]

Description

With the Firebase dynamic link deprecation, the template DeepLinkClient should expose an abstract class that enables easy implementation of the deep link service with another package.

Type of Change

• ✨ New feature (non-breaking change which adds functionality)
• 🛠️ Bug fix (non-breaking change which fixes an issue)
• ❌ Breaking change (fix or feature that would cause existing functionality to change)
• 🧹 Code refactor
• ✅ Build configuration change
• 📝 Documentation
• 🗑️ Chore

[elianortega]

If there is any feedback regarding the naming of the DeepLinkService and the updated folder structure for deep_link_client/, please let me know 🙌

I also ran all the workflows on this PR from my repo.

Finally, what are your thought on providing another implementation of the DeepLinkService? Dynamic links are on their way to be deprecated so I think that providing another implementation using package:app_links or package:uni_links could be a greate addition to the template.

[matiasleyba]

If there is any feedback regarding the naming of the DeepLinkService and the updated folder structure for deep_link_client/, please let me know 🙌

I also ran all the workflows on this PR from my repo.

Finally, what are your thought on providing another implementation of the DeepLinkService? Dynamic links are on their way to be deprecated so I think that providing another implementation using package:app_links or package:uni_links could be a greate addition to the template.

If there is any feedback regarding the naming of the DeepLinkService and the updated folder structure for deep_link_client/, please let me know 🙌

I also ran all the workflows on this PR from my repo.

Finally, what are your thought on providing another implementation of the DeepLinkService? Dynamic links are on their way to be deprecated so I think that providing another implementation using package:app_links or package:uni_links could be a greate addition to the template.

@elianortega I agree that adding a new implementation of deeplinking would be great 👍🏻

app_links seems to be one of the best packages out there and meets the toolkit requirements.
uni_links seems abandoned, I wouldn't use it.

Remember to keep an eye on the package configuration, as we should configure both Android and iOS platforms in the template and also document any necessary configuration needed.

[elianortega]

@elianortega I agree that adding a new implementation of deeplinking would be great 👍🏻

app_links seems to be one of the best packages out there and meets the toolkit requirements. uni_links seems abandoned, I wouldn't use it.

Remember to keep an eye on the package configuration, as we should configure both Android and iOS platforms in the template and also document any necessary configuration needed.

@matiasleyba Yes, I'll send that to another PR after this one gets merged 👍

[matiasleyba]

@elianortega Thanks for addressing the comments, the changes look good! Anyway I would like to hold the merge until we solve a issue we are having with the CI workflows.
Once we solve the issue we will be able to merge the PR, I will keep you posted on the issue.

[elianortega]

No worries @matiasleyba, let me know if I can help with anything 👍

[matiasleyba]

Hi @elianortega, we have been able to resolve the issues with the CI workflows, would you mind updating the PR? Thanks in advance!

[matiasleyba]

Hi @elianortega , reviewing the project again I see that the deep_link_client is very much tied to the email link authentication functionality and not to a deep linking solution as the current navigation approach does not support deep links.
Basically we are using dynamic links because of the email link authentication functionality, but this functionality will not be deprecated, it will be replaced.

Do you think it is still worth making the change, what do you think?

[elianortega]

Hey @matiasleyba sorry for the late reply,

reviewing the project again I see that the deep_link_client is very much tied to the email link authentication functionality and not to a deep linking solution

Yes, the current scope in the toolkit and for this change was just for authentication purposes, but I think adding the DeepLinkClient abstraction is can become a feature for the toolkit. From my experience using the toolkit twice, I ended up adding deep links anyway, as it is a very requested feature for news applications.

But maybe this PR could be the beginning of adding deep link support to the toolkit. To handle this appropriately I'll create a feature request for the toolkit, and resolve the conflicts of this PR.

[matiasleyba]

Hey @matiasleyba sorry for the late reply,

reviewing the project again I see that the deep_link_client is very much tied to the email link authentication functionality and not to a deep linking solution

Yes, the current scope in the toolkit and for this change was just for authentication purposes, but I think adding the DeepLinkClient abstraction is can become a feature for the toolkit. From my experience using the toolkit twice, I ended up adding deep links anyway, as it is a very requested feature for news applications.

But maybe this PR could be the beginning of adding deep link support to the toolkit. To handle this appropriately I'll create a feature request for the toolkit, and resolve the conflicts of this PR.

It would be great to add support for deep linking, but we should address this by changing the navigation approach first, probably using go_router.

[matiasleyba]

@elianortega As the implementation of the DeepLinkClient class is very much tied to Firebase auth I don't see another use case for this class apart from authentication using a link, I would like to know if you find any other use case or a reason to abstract it ?

[elianortega]

If there are no issues with moving towards another navigation approach, I agree that refactoring to use go_router first is the way to go, that would also enable the usage of the built-in deep link handling from go_router for basic navigation. Is this change already considered in the backlog of the toolkit?

I would like to know if you find any other use case or a reason to abstract it?

The only use case I found for the abstraction, is to be able to quickly implement the deep link handling with another package like: app_links or flutter_branch_sdk. In some experiences I've had with the toolkit, even we change to go router, we still used a custom deep link implementation to do extra work with the deep link.

But If the migration to go_router is already in the plan, I think that would cover the must common scenario of navigation to news article.

[matiasleyba]

If there are no issues with moving towards another navigation approach, I agree that refactoring to use go_router first is the way to go, that would also enable the usage of the built-in deep link handling from go_router for basic navigation. Is this change already considered in the backlog of the toolkit?

I would like to know if you find any other use case or a reason to abstract it?

The only use case I found for the abstraction, is to be able to quickly implement the deep link handling with another package like: app_links or flutter_branch_sdk. In some experiences I've had with the toolkit, even we change to go router, we still used a custom deep link implementation to do extra work with the deep link.

But If the migration to go_router is already in the plan, I think that would cover the must common scenario of navigation to news article.

Deeplinking is in the plan, but we don't have an estimate of when we will start working on it.

As for abstraction, if you find it useful in previous implementations, we can continue working on this PR.
I will keep an eye on the PR when it is ready for review.

[matiasleyba]

@elianortega Just a few comments before merge

[elianortega]

@matiasleyba Thanks for the review, I've made the changes, please let me know if you have any other feedback on this one

[matiasleyba]

@elianortega thanks for addressing changes, just one pending before merging.

[elianortega]

@matiasleyba It is true, it was not being used my bad, I removed it