-
-
Notifications
You must be signed in to change notification settings - Fork 916
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
Async navigation cancelation #826
Conversation
Co-authored-by: Joel Denning <joeldenning@gmail.com>
File size impactdist (+7,326 bytes)Overall impact on dist files size
Detailed impact on dist files size
Impact on dist files cache8 files in you users cache are now outdated because their content have changed.
|
@cjpearson @ah3nry can you confirm that the description above, along with the code and tests, fit your needs well? |
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.
Minor question, otherwise looks good.
const unloadPromises = appsToUnload.map(toUnloadPromise); | ||
if (navigationIsCanceled) { | ||
// Change url back to old url, without triggering the normal single-spa reroute | ||
originalReplaceState.call( |
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.
Is it possible that originalReplaceState is null
in any situation here?
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.
Hmm good catch. I think that if someone calls navigateToUrl()
before calling start()
or patchHistoryApi()
that it would be undefined. I'll try to reproduce in a test, then fix to have it default to window.history.replaceState
if originalReplaceState
is not defined.
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.
After writing the test, it didn't throw an error. The reason is that reroute()
doesn't allow you to cancel navigation until you've called start()
, since it short circuits when single spa isn't started. And so this code will never execute until after patchHistoryApi (which is called by start()) is executed. See my upcoming commit in this PR that adds the test.
Going to merge this for the beta release of single-spa@6. Am happy to address any other feedback in future PRs. |
Thanks for taking this over! I started testing with the latest beta today. So far it all looks good, and the feature is working well. |
Resolves #670. I started with the code from @cjpearson in #689 and finished out the implementation, including adding tests.
This is a breaking change, as single-spa events are no longer fired for the reroute caused by cancelation, and also the
navigationIsCanceled
boolean is removed from all custom event detail objects (since it's never true anymore due to events no longer being fired after cancelation occurs). Since it's breaking, I have it merging into the 6.0 branch rather than the master branch.