search

johnpatrickmorgan / FlowStacks

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

`@Environment(\.dismiss)` no longer works when using 0.3.5 #68

Closed rhysm94 closed 6 months ago

rhysm94 commented 6 months ago

Hi there!

Sorry, don't currently have a shareable reproduction case, but the change made in 0.3.5 seems to have broken the SwiftUI native @Environment(\.dismiss) action. The view that's presented by a TCARouter does not actually dismiss correctly. In my TCA state, nothing changes either when I press a button that calls the Environment's dismiss in a view.

I'm using TCACoordinators, but the issue doesn't appear to be specifically with TCACoordinators – if I pin my app to FlowStacks 0.3.4, everything works great. If I pin to 0.3.5, it breaks and @Environment(\.dismiss) stops working.

The issue affects both iOS 16 and iOS 17 – I've tried it on both iOS 16.4 and iOS 17.0.1/17.2 sims, and the issue persists. Given the only other non-iOS 17 change that exists is wrapping a screenView in a ZStack, I wonder if that's the particular issue I'm encountering. I'm just wondering if the screenView needs the whole @Environment propagating to it perhaps? I dunno, I'll give that a try and PR if it fixes things!

I'll try and get a reproduction case up soon as well, but I thought I'd better report the issue sooner!

johnpatrickmorgan commented 6 months ago

Thanks for finding this @rhysm94! I did a quick test and it seems that wrapping screenView in a ZStack is indeed the culprit. Here's the context for that change - it seems the 'unintended consequences' I feared have come to roost! Given that the reason for adding the ZStack was a lot more niche than the use of @Environment(\.dismiss), and can be worked around in user code, I think removing the ZStack would be the best course of action, but I'll have a play and see if I can find a way to keep both fixes.

rhysm94 commented 6 months ago

Sorry I didn’t quite get round to producing a repro. case for you this evening, but glad it hopefully wasn’t too difficult to find. Shame it’s not possible to propagate a dismiss action through the environment, as I imagine that would be a fix, but \.dismiss is only KeyPath, not a WritableKeyPath 😭

johnpatrickmorgan commented 6 months ago

I've removed the ZStack wrapper in version 0.3.8. Thanks @rhysm94!

rhysm94 commented 6 months ago

I've removed the ZStack wrapper in version 0.3.8. Thanks @rhysm94!

Fab, thank you! Much appreciated 😄