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

Handling failed single-page app navs #47

Closed
domenic opened this issue Feb 25, 2021 · 3 comments · Fixed by #60
Closed

Handling failed single-page app navs #47

domenic opened this issue Feb 25, 2021 · 3 comments · Fixed by #60
Labels
change A proposed change to the API, not foundational, but deeper than the surface

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 25, 2021

The design of navigation interception, especially after #46 lands, leads to a novel situation. Namely, you can perform a single-page app navigation which fails, by passing a rejected promise to event.respondWith(). A somewhat-realistic example might be:

appHistory.addEventListener("navigate", e => {
  e.respondWith((async () => {
    const response = await fetch("https://nonexistant.invalid/some-data.json");
    // ... etc ...
  })());
});

// This will get intercepted and converted to a failure.
location.href = "/foo";

The current plan as of #46 is that, because of the issues discussed in #19, all such navigations will synchronously succeed, but then asynchronously fail. Roughly:

  1. location.href, the URL bar, etc. immediately start pointing to /foo, as if you'd done history.pushState(null, null, "/foo").
  2. appHistory.current, as well as the underlying session history model, gets pointed to a new history entry for /foo.
  3. Any forward history entries get thrown away.
  4. But then, once the browser sees that the promise is rejected, it "rolls back" the navigation:
    1. It sets location.href, the URL bar, etc. back to the original URL that initiated the navigation.
    2. appHistory.current, as well as the previous session history model, navigates back to the previous session history entry.
    3. The history entry for /foo created in step (2) gets thrown away.
    4. The forward history entries thrown away in (3) remain dead.

Is this the right model for when the navigate handler tells us the navigation failed?

The main alternative, I think, is to do a lot less. I.e., do not try to provide a rollback; leave location.href, the URL bar, etc. on /foo. Instead just signal to the app (e.g. with an event on window.appHistory and/or window.appHistory.current) that the promise rejected, and have the app handle that. The app could do its own rollback, perhaps. Or it could display a literal error page, like it might for a server-side 404.

I'm kind of leaning toward this do-less model. What do people think?

The other alternative would be to go the route of (2) in #19 (comment), which would allow us to delay all the updating stuff until we're sure the promise fulfills. Then there would be no navigate-then-rollback; we'd just never move at all in a failure case.

@domenic domenic added the change A proposed change to the API, not foundational, but deeper than the surface label Feb 25, 2021
domenic added a commit that referenced this issue Feb 25, 2021
This includes ones generated by intercepting the navigate event with event.respondWith(). However, the promise passed to event.respondWith() is still useful for signaling the navigation's successful or unsuccessful completion. That is used to fuel browser UI, such as the loading spinner, as well as promises and events. And a rejected promise passed to event.respondWith() will roll back the navigation.

Closes #19. Closes #44. Still some discussion to be had on the failure case in #47.
@tbondwilkinson
Copy link
Contributor

I lean towards the do-less model as well. It seems to me that if we can mirror cross-document navigations, we get some benefit in a simplified model. I do think we need new hooks though, like a navigatefailed event or something, as you mention.

@Yay295
Copy link

Yay295 commented Feb 26, 2021

Would a .rollback() method on the navigation event be possible? I agree that not rolling back by default is probably the better idea, but I also think a lot of people are going to want to roll back, so it would be convenient to have an easy way to do it.

domenic added a commit that referenced this issue Mar 5, 2021
This removes some of the complexity introduced in 9d750db. Closes #47.
@domenic
Copy link
Collaborator Author

domenic commented Mar 5, 2021

In #60 I added some code examples to use navigateerror to rollback (using appHistory.back()) or display an error. It seems pretty good to me to start, although as discussed offline with @tbondwilkinson we might want to make the tie between the failed navigation and the failure handling stronger, instead of having them use two different events.

domenic added a commit that referenced this issue Mar 5, 2021
This removes some of the complexity introduced in 9d750db. Closes #47.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change A proposed change to the API, not foundational, but deeper than the surface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants