[css-view-transitions-1] Fix some promise timing, and avoid duplicate unhandled rejections

[jakearchibald]

In the current spec, this will cause three unhandled rejections:

const transition = document.startViewTransition(() => {
  doesNotExist();
});

As it will cause transition.updateCallbackDone, transition.ready, and transition.finished to reject with the same error.

This PR always marks transition.finished as handled, as it only ever rejects for the same reason as transition.updateCallbackDone. It marks transition.ready as handled if it's rejected with the same reason as transition.updateCallbackDone.

[jakearchibald]

Seems like bikeshed's markdown parser doesn't support emphasis with _. Just a formatting fix.

[jakearchibald]

No normative change here, just using more standard spec prose.

[jakearchibald]

This is technically a normative change, but it doesn't change any currently observable behaviour. It protects us from possible race conditions in future.

[jakearchibald]

Again, technically a normative change, but no observable behaviour change.

[jakearchibald]

https://jakearchibald.com/2014/resolve-not-opposite-of-reject/ 😄

[vmpstr]

This would only be a duplicate if update callback done promise is not handled. If it is handled, but the finished promise is not handled, then should that still cause an unhandledrejection?

In other words, I feel like we shouldn't try too hard to prevent duplicate unhandledrejections, since these are different promises that would both be not handled if rejected

[jakearchibald]

Imagine code like this:

async function attemptTransition() {
  const transition = document.startViewTransition(callback);

  try {
    await transition.updateCallbackDone;
  } catch (err) {
    reportBadError(err);
    return;
  }

  try {
    await transition.ready;
    // Do some JS animations here
  } catch (err) {
    reportSkippedTransition(err);
    return;
  }

  try {
    await transition.finished;
    // Do some cleanup
  } catch (err) {
    reportCleanupError(err);
    return;
  }
}

If the update callback throws, the above code will result in two identical unhandled rejections. Although the updateCallbackDone promise is handled, the function exits, leaving ready and finished unhandled. That doesn't seem useful, and annoying to work around. So, my goal in this PR is to avoid unhandled rejections on ready and finished when the rejection would be the same as updateCallbackDone.

Note that as soon as the developer does:

await transition.finished;

…they're creating a new unhandled promise. This PR only impacts cases where the developer doesn't do anything with the promises.

[vmpstr]

I was imagining the case where updateCallbackDone is listened to by some app state monitor, to make sure that all of the proper dom modifications take place, whereas finished is listened to by some logging / metrics subsystem to just report failures. In that case, suppose that updateCallbackDone rejects and is handled appropriately by the app state monitor. However, the logging system doesn't handle it (maybe as an oversight) but it does not result in unhandled rejection because updateCallbackDone handled it.

I think my case is fairly contrived, but I'm just wary of handling rejections for promises that seem different enough.

WDYT?

[jakearchibald]

Yeah, I see what you're saying. But it doesn't feel that different to what would happen if both systems were listening to updateCallbackDone, which I think it more likely in this case (finished only ever rejects if updateCallbackDone rejects).

• The app state handles updateCallbackDone.
• The logging system intends to handle updateCallbackDone, but fails to for some code path.

There's no unhandled rejection from updateCallbackDone, because the app state already handled it.

I'll ask Domenic for a second opinion on this though.

Tangent: I kinda think unhandled rejections in general are a bad design. They seem to be built around single consumer.

[jakearchibald]

Domenic likes the pattern in this PR (he did a similar thing in the navigation API) #8455 (comment).

But I'm not opposed to take it to the group internally, or CSSWG, if you want more eyes on it.

[vmpstr]

Likewise here, it seems that this is only a duplicate if update callback done promise is not explicitly handled? Or am I misunderstanding?

[vmpstr]

I feel like this is an extremely subtle point which I wouldn't have known if you didn't link to the your 2014 article.

"resolve the promise with the result of reacting to callbackPromise". This does, in fact, mean reject the promise if callbackPromise was rejected, right?

I'm not sure what race conditions you're referencing below, but I think it would be better to be explicit here "reject if rejects, resolve if resolved" type of language

[jakearchibald]

"resolve the promise with the result of reacting to callbackPromise". This does, in fact, mean reject the promise if callbackPromise was rejected, right?

Yeah, the process of react is very similar to promise.then(), as in, if you don't react to the reject case, then the rejection is passed through.

I'll add a note to make this clear.

The only reason the fulfilled case is handled is to ensure that updateCallbackDone fulfills with undefined rather than whatever the callback returns.

[jakearchibald]

I'm not sure what race conditions you're referencing below

We didn't have any in the spec right now, but since our intention was to have updateCallbackDone follow callbackPromise, except resolving with undefined, it seems better to spec it with that intent.

Here's the difference between the two patterns: https://jsbin.com/butulov/edit?js,console