router: change URL in between navigating & navigated #4974
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
To reviewer: recommend turning on 'ignore white space change' option. |
psybuzz
approved these changes
May 17, 2021
There was a problem hiding this comment.
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.
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this change, we now modify the URL before
navigatedis 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
navigatedaction 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
navigatedisfired, 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
navigatedto really reallymean, 'hey, the navigation is now complete and everything is settled'.
Tested this change internally.