-
Notifications
You must be signed in to change notification settings - Fork 43
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
Post Refactor update to session coordinator #1518
base: main
Are you sure you want to change the base?
Conversation
var opts = NavOptions() | ||
opts.joinParams = .some([ "_interface": .object(object: LiveSessionParameters.platformParams)]) | ||
opts.action = .replace | ||
let _ = try await client.navigate(next.last!.url.absoluteString, opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a problem here, core now runs it's own even loop and will take patch and redirect events, these are currently not handled if they are not triggered by a client action. Those events are all pushed through SimpleEventHandler.viewReloadSubject
. We need to watch for events that are emitted by server-side events and rectify the navigation path.
the other option is just to call the appropriate client navigation functions wherever we would previously just modify the navigation path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following the specific issue here. Can you provide more detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously the swiftui client listened on the live view channel for live_reload
and other push_navigation
type events, then it would modify the LiveSessionCoordinator.NavigationPath
and all navigation would happen downstream of checking for changes in the scan
block like we do here.
Now that push navigation events are handled opaquely inside core, we need to update the LiveSessionCoordinator.NavigationPath
in response to internal events. While the code provided here works, the NavigationPath
should be controlled by core, which is the opposite of how it works now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and yes, I agree
here are speculative changes that would need to be made to the swiftui client if my refactor landed in it's current state.