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

Make any navigation cancel ongoing navigate events #104

Merged
merged 2 commits into from
Apr 23, 2021

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Apr 23, 2021

(This is in the broad sense of "navigation", which includes the navigate algorithm, history traversal, and pushState()/replaceState(). I.e., anything that would fire a navigate event, even if it doesn't.)

Discussed in #96 (comment).

Note that this doesn't prohibit calling appHistory.navigate() inside a navigate handler; we want to something more sophisticated for that, with nesting level detection. Instead this ensures that we don't get into a weird situation where

appHistory.addEventListener("navigate", e => {
  if (e.destination === url1) {
    const p2 = appHistory.navigate(url2);
  }
  e.respondWith(Promise.resolve());
});
const p1 = appHistory.navigate(url1);

ends up on url1 because both navigations go through and url2 finishes first. Instead the navigation to url2 goes through and cancels the ongoing navigation to url1, i.e. the second one "wins", which is more what you would expect.


Preview | Diff

(This is in the broad sense of "navigation", which includes the navigate algorithm, history traversal, and pushState()/replaceState(). I.e., anything that would fire a navigate event, even if it doesn't.)

Discussed in #96 (comment).
I.e., make sure that the rest of the algorithm doesn't think we called respondWith(). This is important for #96.
@domenic domenic merged commit ed9aa12 into main Apr 23, 2021
@domenic domenic deleted the navigate-during-navigate-2 branch April 23, 2021 19:10
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.

2 participants