Milad-Akarie / auto_route_library

Flutter route generator
MIT License
1.5k stars 375 forks source link

[WIP] - Introduce DeeplinkTransformer #1923

Closed wer-mathurin closed 1 month ago

Milad-Akarie commented 1 month ago

Hey @wer-mathurin Thanks for your efforts, this's exactly what I had in mind except for one detail, why do you think the return type of DeeplinkTransformer should be a FutureOr?

wer-mathurin commented 1 month ago

@Milad-Akarie There are usecases where we just need to remove the path, so just synchronous code.

But there are other usecases where we need to perform some logics other than removing the path prefix that need to be asynchronous.

Best practices suggest do to it in a controllers or something, but to not force the user to follow them.

One example would be, the developers need to validate something store in the SharePreferences before doing the path transformation.

wer-mathurin commented 1 month ago

@Milad-Akarie any plan to merge it soon?

This is a good addition to the library :-)

Milad-Akarie commented 1 month ago

Hey @wer-mathurin sorry for the delay, I'm really swamped these days. I think this's ready to be merged, but I got some minor changes that I'm willing to make my self, including changing the return type to Future<Uri> instead of FutureOr<Uri> because I've recently learnt that FutureOr shouldn't be used in such cases ( should be avoided in general ) from a member of the dart team and it only exists as a back compatiablity workaround for some old implementation before to dart 2.

just so you understand my concern, when you return T as FutureOr<T>, T will be wrapped inside a future internally, so it's never a synchronous operation. on the other hand if we change the return type to Future and return a value inside of SynchronousFuture(transformedUri) then we have a synchronous return type.

Of course we can keep FutureOr and utilize the SynchronousFuture, but most users think returning a T directly will happen synchronously so won't bother use SynchronousFuture

from SynchronousFuture docs

⚠ This class is useful in cases where you want to expose a single API, where you normally want to have everything execute synchronously, but where on rare occasions you want the ability to switch to an asynchronous model.

wer-mathurin commented 1 month ago

I've recently learnt that FutureOr shouldn't be used in such cases ( should be avoided in general ) from a member of the dart team and it only exists as a back compatiablity workaround for some old implementation before to dart 2.

Good to know.

Does it mean you will change the return type of the API expose by the library as well?

This would be a major version increase, because this is a breaking change.

BTW, if you just comment on the pull request, I can do the change for the other little things as well. I think, I also should update the deeplink section in the readme.

Milad-Akarie commented 1 month ago

Eventually, yes, but for now its better if we introduce this without FuturOr.

I'll handle the rest

wer-mathurin commented 1 month ago

@Milad-Akarie : DONE ATTEMPT in Changelog.md