-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
router: change URL in between navigating & navigated #4974
Conversation
To reviewer: recommend turning on 'ignore white space change' option. |
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.
Thanks for testing it internally. I tested it in OSS and looks good
navigationOptions: { | ||
replaceState: false, | ||
}, | ||
navigateAndExpect(activeRoute, { |
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.
Based on this call, the type for the first parameter to navigateAndExpect
on L645 should be Navigation | Route
, not Navigation
.
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.
Fixed; thanks for catching it.
With this change, we now modify the URL before `navigated` is fired so all the downstream can send request right away without having to confirm that the route change is indeed reflected in the URL bar. Context: previously, our router modified the URL only when the `navigated ` action is fired. Often, this action is listened to by many views/effects so they can do a cleanup or a data fetch for the new route. Now, because the URL was only modified after `navigated ` is fired, there could be a timing issues where, depending on the order of execution, we can be making requests to the unmodified URL, causing issues. Now, we are changing the semantics of the `navigated` to really really mean, 'hey, the navigation is now complete and everything is settled'. Tested this change internally.
Tested once more; since unclear whether this will break our odd variant, I will merge this, sync it, then simmer it so it can be reverted in isolation. |
With this change, we now modify the URL before
navigated
is fired soall the downstream can send request right away without having to confirm
that the route change is indeed reflected in the URL bar.
Context: previously, our router modified the URL only when the
navigated
action is fired. Often, this action is listened to by manyviews/effects so they can do a cleanup or a data fetch for the new
route. Now, because the URL was only modified after
navigated
isfired, there could be a timing issues where, depending on the order of
execution, we can be making requests to the unmodified URL, causing
issues.
Now, we are changing the semantics of the
navigated
to really reallymean, 'hey, the navigation is now complete and everything is settled'.
Tested this change internally.