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

Identify stores based off state identity #3464

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

stephencelis
Copy link
Member

@stephencelis stephencelis commented Oct 23, 2024

Potential fix for #3451.

Draft for now. We've talked about this in the past but still don't know the ramifications of it all. It does seem like it should be a good thing, but I'm worried about unforeseen consequences, so we should vet them out.

@stephencelis stephencelis marked this pull request as ready for review November 12, 2024 05:26
@BastianKusserow
Copy link

Hey, sorry to bother but is there any news on this? This unfortunately stops us from using alerts in UIKit at all, as they just show once and destination state is not reflected correctly :(

@acosmicflamingo
Copy link
Contributor

If it helps, I've been using the changes in this PR to unblock my UIKit development and haven't seen issues from this.

@BastianKusserow
Copy link

@acosmicflamingo did you fork the repo or did you directly checkout the branch?

@acosmicflamingo
Copy link
Contributor

@BastianKusserow I have my own fork because it makes it really easy for me to debug issues, upstream any fixes I find, and I can also slap on a public modifier here and there 😉

@nesium
Copy link

nesium commented Mar 28, 2025

This fix does not work if the initial state is non-nil. E.g. in the attached project in #3636 change the code…

// RootFeature.swift::14
@ObservableState
struct State {
-  @Presents var route: Route.State?
+  @Presents var route: Route.State? = .feature1(.init(id: .a))
}

The ViewController with the title "Feature 1 ('a')" will then be presented on launch. Dismiss it, tap "Present Feature 1.b" and dismiss it again. The effects of the second VC are not cancelled and the route is not reset to nil.

@stephencelis stephencelis marked this pull request as draft April 2, 2025 20:59
@stephencelis
Copy link
Member Author

@nesium Thanks for the counterexample! We're still not sure of the validity of incorporating state identity into the store, but also haven't had time to vet it, so going to leave this as a draft till we can revisit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants