appHistory.transition.finished/navigatesuccess/navigateerror can be fired before finished promise

[domenic]

In traversal cases, it's possible for the promise passed to transitionWhile() to settle within a single microtask, while it takes a task to actually perform the traversal.

We've accounted for this partially, in the case of the promise returned by appHistory.back().finished, by using the did finish before commit technique to delay the finished promise until after the commit happens (and the committed promise fulfills.

But I believe we are not doing anything to prevent early firing of navigatesuccess/navigaterror, or early resolution of appHistory.transition.finished.

This will similarly impact #197, i.e. we should not do focus reset or scroll restoration too early in these cases.

We need to generalize the mechanism for delaying finish (in all its forms) until after commit.

[domenic]

I've written some WIP tests and this isn't really observable in the way I thought it was. Because events happen immediately and promise handlers are always microtask-delayed, you would expect that navigatesuccess is fired before the finished promise fulfills. I.e. even if the implementation code does (the C++ equivalent of)

resolveFinishedPromise();
fireNavigateSuccessEvent();

then the following JavaScript code

appHistory.addEventListener("navigatesuccess", () => console.log("navigatesuccess"));
appHistory.back().finished.then(() => console.log("finished promise"));

will still log navigatesuccess, then finished promise. So in all cases we expect navigatesuccess to fire before finished fulfills, and thus the fact that sometimes it does so via the route described above is... fine?

It all still seems very fragile... investigating...

[fabiancook]

Until all promises passed to transitionWhile() have settled

When writing an implementation I hadn't come across this specific requirement, and instead had finished be settled only after the commit has happened, or the transition was aborted in some way.

Implemented somewhat like:

try {
  doEverythingBeforeCommit();
  commit();
  resolveCommitted();
  doEverythingAfterCommit();
} catch (error) {
  fireNavigateErrorEvent();
  rejectCommittedIfNotSettled(error);
  rejectFinished(error);
} finally {
  fireNavigateSuccessEventIfOk();
  resolveFinished();
}

In node + deno I found that if the promise provided to transitionWhile did reject, and we then waited before getting to finally, we would get an uncaught rejection error, to resolve this, we would need to aggregate these rejections individually and throw if we have a caught error before moving on step to another step.

[domenic]

committed is for when the commit has happened; finished is for after all the transition promises have settled.

[domenic]

I've found a couple of fixes we'll need to make in the spec in this area; see here for the corresponding Chromium changes which will soon be transferred over.