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

Fix NavigationView broken state after re-render #259

Merged
merged 1 commit into from
Aug 15, 2020

Conversation

MaxDesiatov
Copy link
Collaborator

After #241 was merged, NavigationView started using @ObservedObject instead of @State for its destination storage. This uncovered a bug where @ObservedObject properties that have their values provided in-place lost their subscriptions after a re-renderer. This manifested in navigation links not working in the demo after any scene phase changes that trigger a re-render. This was caused by a new NavigationContext being created on every render, but a corresponding MountedCompositeElement instance kept dangling subscription to the old NavigationContext instance, while new subscriptions weren't created.

This PR splits subscriptions on MountedCompositeElement into separate "transient" and "persistent" arrays. Persistent subscriptions are set up once when an element is mounted, and are automatically cleaned up when the element is unmounted. Scene phase and color scheme subscriptions on MountedApp are now primary examples of these persistent subscriptions, but @StateObject is another future use case to be implemented in a separate PR. Transient subscriptions are recreated on every re-render to allow new @ObservedObject instances to establish their subscriptions.

NavigationView in the demo still resets to the empty destination view when the scene phase observer is triggered, but NavigationLink selections are fixed now. Resetting to the empty destination view on scene phase changes will be fixed when @StateObject implementation is available in a separate PR.

@MaxDesiatov MaxDesiatov added the bug Something isn't working label Aug 14, 2020
Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

LGTM! Why not make all the subscriptions persistent?

@MaxDesiatov
Copy link
Collaborator Author

They were persistent initially, but as I mentioned in the description, when re-rendered new ObservableObject instances are created, which invalidates old subscriptions.

@MaxDesiatov MaxDesiatov merged commit b9f5ef0 into main Aug 15, 2020
@MaxDesiatov MaxDesiatov deleted the separate-subscriptions branch August 15, 2020 17:07
@j-f1
Copy link
Member

j-f1 commented Aug 15, 2020

when re-rendered new ObservableObject instances are created, which invalidates old subscriptions.

Is that intended behavior or a deviation from SwiftUI?

@MaxDesiatov
Copy link
Collaborator Author

That's intended and is the reason for adding @StateObject this year to the upstream API, which persists across re-renders.

@MaxDesiatov
Copy link
Collaborator Author

MaxDesiatov commented Aug 15, 2020

BTW, if the instance is created outside of the element hierarchy or in an ancestor element that's not re-rendered, it persists. That was the recommended way to work around it, but NavigationView needs to instantiate the context on its own.

Also, more details are available here: https://stackoverflow.com/questions/62544115/what-is-the-difference-between-observedobject-and-stateobject-in-swiftui

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants