Better example and API for rolling back failed navigations

[domenic]

In an offline discussion @atscott pointed out that the rollback example at https://github.com/WICG/app-history#example-handling-failed-navigations does the wrong thing in any cases besides a simple non-replace forward navigation. Some cases to consider would be:

• When a back navigation results in a rejected promise and corresponding navigateerror event (e.g., after logging out, trying to go back to a login-guarded page, which the router rejects). Then, the correct way of rolling back the navigation is actually to do appHistory.back()
• When you skip multiple entries, e.g. if you go back two steps. Then the correct way of rolling back the navigation would involve some appHistory.goTo() with a saved key.
• When you perform a replace navigation that fails. Then the correct way of rolling back would probably be another replace, back to the old URL/state.

Also, he mentioned that such rollbacks likely want to communicate to the router that they are a rollback, so it can treat them specially. This is likely best done via a navigateInfo parameter.

There are a few ways one could envision solving this. So far I've come up with:

• Add an event.previous AppHistoryEntry, to the navigateerror event. (And to the navigatesuccess event?) Then rollbacks would be somewhat manual, e.g. await appHistory.goTo(event.previous.key, { navigateInfo }).
  • The failed-replace case would be especially tricky. I think you could detect it if we follow through on Update vs. replace, entry mutability, and keys #7 (so that you'd have event.previous.key === appHistory.current.key, even though event.previous.id !== appHistory.current.id). But it'd need its own special-case coding, and it'd be easy to forget.
• Add a "do what I mean" event.rollback({ navigateInfo }) to the navigateerror event.
• Do both of these, so that event.rollback() is explained as a helper function that does some of the trickier rollback logic for you, but you can always construct your own.
• Some variant of these that is not tied to the navigateerror event, e.g. we could just always have appHistory.previous available (which might also help with Retrieving data from replaced entries #41).

Any thoughts, or other ideas?

[tbondwilkinson]

Tricky!

At the time navigateerror is sent would we say the navigation is "done" at that point? Or immediately after the navigateerror is resolved?

I like event.rollback({navigateInfo}), in general. I think that's ergonomic and a pretty common use case.

But I also like the idea of knowing more information about an ongoing navigation. I'm not sure it should be AppHistoryEntry but maybe the router wants to know "what is the current navigation that's happening, what is the destination information (what the entry is after the navigation resolves, to the best of the appHistory's knowledge) and the source information (what was the entry at the start of the navigation, in case that changes something about how the router handles a failure). As part of that, you might want to know what type of navigation is taking place (push, replace, or directional go()).

[atscott]

But I also like the idea of knowing more information about an ongoing navigation. I'm not sure it should be AppHistoryEntry but maybe the router wants to know "what is the current navigation that's happening, what is the destination information (what the entry is after the navigation resolves, to the best of the appHistory's knowledge) and the source information (what was the entry at the start of the navigation, in case that changes something about how the router handles a failure). As part of that, you might want to know what type of navigation is taking place (push, replace, or directional go()).

The Angular router also has this sort of information tracked in the current navigation. It could be useful if the app history API had similar information available. It's not currently used to determine how to handle rollbacks of rejected navigations but as I mentioned, we don't currently handle rollbacks correctly. There's a proposed change that would enable this and does have to look at the navigation trigger to determine the correct strategy for rolling back: https://github.com/angular/angular/pull/38884/files#diff-03cb1ea13f0b5890eb24ce6fef7a42eed85429da0a90cdb48ab06b58c30cb1c1R1433-R1441. This kind of logic might not be necessary in the app history world if you could navigateTo(appHistory.previous.key) and it just does the right thing for replace vs push vs pop/back.

[domenic]

But I also like the idea of knowing more information about an ongoing navigation. ... what is the current navigation that's happening, what is the destination information (what the entry is after the navigation resolves, to the best of the appHistory's knowledge) and the source information (what was the entry at the start of the navigation, in case that changes something about how the router handles a failure)

I think this pushes toward appHistory.previous, because "the ongoing navigation" is actually appHistory.current. Then:

• If appHistory.current.finished is false, there is a respondWith()-generated navigation "ongoing". In that case:

  • appHistory.current is about the "destination"
  • appHistory.previous is about the "source"

This kind of logic might not be necessary in the app history world if you could [edited] appHistory.goTo(appHistory.previous.key) and it just does the right thing for replace vs push vs pop/back.

Again the tricky thing here is replace. The key-based model works fairly poorly with replaces as we've currently specced goTo(). You'd have to do

if (appHistory.previous.key === appHistory.current.key && appHistory.previous.id !== appHistory.current.id) { // see #7
  appHistory.navigate(appHistory.previous.url, { state: appHistory.previous.state, replace: true });
} else {
  appHistory.goTo(appHistory.previous.key);
}

(Note: using post-#84 names.)

Again, maybe we could package that up into appHistory.rollback() or something, but that seems like we're papering over the problem.

There are a few ways of making this suck less that I can think of:

• appHistory.goTo() accepts an AppHistoryEntry either instead of, or in place of, a key.
• appHistory.goTo() switches to accepting IDs instead of keys.

I'd very much appreciate thoughts on this since I'm still a little shaky on how to think about when people use goTo() and keys/IDs.

[tbondwilkinson]

What about an alternate API approach:

class AppHistoryTransition {
  type: 'replace'|'push'|'go';
  previous: AppHistoryEntry;
  current: AppHistoryEntry;

  rollback(): Promise;
}

Expose something like this as appHistory.currentTransition, and the rollback() method will throw an error if the transition becomes invalidated (because some other history transition occurred.)

I think I'm hesitant to just say "let's let people do whatever they want" because I do think that rollback() is 90% of use cases, and the other 10% are probably better reflected as "updates" rather than reversions to some previous state.

[domenic]

Hmm. I'm trying to think of how that compares with just putting everything on appHistory, i.e.: appHistory.previous, appHistory.current, appHistory.rollback().

• (+) It gives you more information about the ongoing transition with appHistory.currentTransition.type, which could be cool.
• (-) It has a redundancy with appHistory.currentTransition.current equaling appHistory.current, which is kind of confusing.
• (Neutral?) It only lets you call appHistory.currentTransition.rollback() during the transition, instead of any time. Probably that's more appropriate?

[tbondwilkinson]

I was bothered by the redundancy too, it could be dropped entirely though. What about:

class AppHistoryTransitionInfo {
  type: 'replace'|'push'|'go';
  initiated: timestamp;
  previous: AppHistoryEntry;
}

And then add a rollback method to appHistory:

appHistory.rollback(appHistory.transitionInfo);

This should throw an error if the transitionInfo was not the last transition (and no additional transition is currently taking place), or the current transition.

We could expose transitionInfo directly on appHistory, or additional on navigationerror/success

appHistory.addEventListener('navigateerror', (event) => {
  if (...) {
    appHistory.rollback(event.transitionInfo);
  }
});

[atscott]

Not to add too much craziness here without any helpful solution suggestions, but I just want to make sure this scenario is considered when thinking about rollback:

0. Application is happily sitting on the homepage /
1. Navigate to /a
2. Framework handles the 'navigate' event using respondWith so it can run its async guards and such
3. Framework determines it needs to redirect this route so it triggers a new navigation using replace (this part may change depending on what's decided for async redirects) to /b
4. Framework handles the new 'navigate' event with respondWith as well, then runs its guards and determines that the navigation should be rejected (and rejects the promise).

What does the rollback do in this scenario? We'd really want to roll all the way back to / rather than /a since we never really allowed /a to settle/proceed.

[tbondwilkinson]

If the navigate to /a is cancelled, it could also be rolled back to /. Then the navigate to /b would sit on top of / rather than as a replace of /a.

…

On Tue, Mar 30, 2021 at 8:02 PM Andrew Scott ***@***.***> wrote: Not to add too much craziness here without any helpful solution suggestions, but I just want to make sure this scenario is considered when thinking about rollback: 1. Application is happily sitting on the homepage / 2. Navigate to /a 3. Framework handles the 'navigate' event using respondWith so it can run its async guards and such 4. Framework determines it needs to redirect this route so it triggers a new navigation using replace (this part may change depending on what's decided for async redirects) to /b 5. Framework handles the new 'navigate' event with respondWith as well, then runs its guards and determines that the navigation should be rejected (and rejects the promise). What does the rollback do in this scenario? We'd really want to roll all the way back to / rather than /a since we never really allowed /a to settle/proceed. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#86 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASEG7W2EKVYWRYM35FQVALTGJYDRANCNFSM4ZZ757YA> .

[domenic]

I was bothered by the redundancy too, it could be dropped entirely though. What about:

Here's my variant of this, merging a bit with your previous proposal:

// can be null if no transition is ongoing
appHistory.transition;

// "replace", "push", or "go" (maybe "traversal" instead of "go"?)
appHistory.transition.type;

// timestamp
appHistory.transition.initiated;

// AppHistoryEntry
// maybe "source" instead of "previous"?
appHistory.transition.previous;

// Only works while the transition is active, i.e.
// you cannot save `const oldTransition = appHistory.transition` and call `oldTransition.rollback()`
// unless `appHistory.transition === oldTransition`.
appHistory.transition.rollback({ navigateInfo });

The transition gets cleared after navigatesuccess/navigateerror finish firing all their handlers.

Not to add too much craziness here without any helpful solution suggestions, but I just want to make sure this scenario is considered when thinking about rollback:

Great to make this concrete. As Tom says, I think this would work best if you rolled back both transitions. With the above API, that'd look something like:

appHistory.addEventListener("navigate", e => {
  e.respondWith((async () => {
    const result = await allowedToGoTo(e.destination.url);
    if (result.type == "redirect") {
      appHistory.transition.rollback();
      appHistory.navigate(result.destinationURL, { state: result.destinationState });
    } else if (result.type === "disallow") {
      throw new Error("Bad!");
    }
  })());
});

appHistory.addEventListener("navigateerror", e => {
  appHistory.transition.rollback();
});

I think this works...

[tbondwilkinson]

+1 that looks good to me!