johnpatrickmorgan / FlowStacks

FlowStacks allows you to hoist SwiftUI navigation and presentation state into a Coordinator
MIT License
783 stars 56 forks source link

Who is responsible for removing the node from the Routes when the sheet is dismiss from a swipe ? #69

Closed lionel-alves closed 6 months ago

lionel-alves commented 6 months ago

Hi! This lib is still the best way to handle navigation using SwiftUI!

When a sheet is dismiss with a swipe, it takes a really long time for the routes to actually be updated, ~1 sec after the sheet is completely gone, so if you tap quickly on something that push it can do nothing or crash (if the sheet didn't have embedInNavigationView: true)

Not a lot we can do here since the onDismiss returns pretty late. But this got me thinking, looking at the code I don't understand where the routes are actually updated in that case, no ones is calling dismiss to actually remove this screen from the routes. It does disappear so it is done somewhere.

Thanks for your help @johnpatrickmorgan

PS: I actually re-did a benmark of all the possible solutions to handle navigation using SwiftUI, and this lib is truly the only nice way! 😂

johnpatrickmorgan commented 6 months ago

Hi @lionel-alves, thanks for your kind words!

looking at the code I don't understand where the routes are actually updated in that case, no ones is calling dismiss to actually remove this screen from the routes. It does disappear so it is done somewhere.

The binding that is passed to the sheet modifier has its setter invoked with a false value when a sheet is dismissed, so at that point we can update the routes array:

https://github.com/johnpatrickmorgan/FlowStacks/blob/9629bd329146d4ba476bfdb50d293acbf8ba3f5d/Sources/FlowStacks/Node.swift#L23-L27

Unfortunately the binding's setter is invoked at the very end of the dismiss animation, once the sheet is fully dismissed. In my testing, it updated pretty quick but if I was fast enough I was indeed able to interact with the screen before the routes array had been updated.

In theory, the library could make an environment value available to indicate if the screen in question is at the top of the stack, so that you could disable certain interactions until the routes array has been updated. That seem like it might be helpful.

lionel-alves commented 6 months ago

Got it! Thanks for the explanation @johnpatrickmorgan. The environment value could be useful indeed 👍