Fix Promise.prototype.finally to not incur extra ticks #1118
Conversation
|
@ljharb Please review |
| @@ -39015,57 +39030,11 @@ <h1>Promise.prototype.finally ( _onFinally_ )</h1> | |||
| <p>When the `finally` method is called with argument _onFinally_, the following steps are taken:</p> | |||
| <emu-alg> | |||
| 1. Let _promise_ be the *this* value. | |||
| 1. If Type(_promise_) is not Object, throw a *TypeError* exception. | |||
| 1. If IsPromise(_promise_) is *false*, throw a *TypeError* exception. | |||
There was a problem hiding this comment.
This isn’t going to work; finally must work with thenables.
There was a problem hiding this comment.
Why is that? Promise.prototype.then does this check as well. Shouldn't Promise.prototype.then work with thenables as well?
There was a problem hiding this comment.
Because of subclassing, that check must be restricted to .then so that it can be overridden.
| <p>The `length` property of a Catch Finally function is *1*.</p> | ||
| </emu-clause> | ||
| 1. Let _resultCapability_ be ? NewPromiseCapability(_C_, _promise_). | ||
| 1. Return PerformPromiseThen(_promise_, _onFinally_, _onFinally_, _resultCapability_). |
There was a problem hiding this comment.
The fact that this does not observably call .then immediately means that a subclass has to override .finally - with the current design, it only has to override .then (which is a requirement)
There was a problem hiding this comment.
I think that makes sense because in essence then and finally are different. Could you please clarify why you think otherwise? The current design is based on what I consider to be a workaround instead of being an exact expression of what finally means conceptually, hence the extra ticks that finally doesn't conceptually require.
I think finally should call then only if the behavior expressed by then is part of finally's behavior. In my opinion it's not fully; they are similar only to a certain degree. Perhaps listing the pros and cons and the possible trade-offs and analyzing which one is better would help making the right choice.
There was a problem hiding this comment.
What I'm saying applies to Promise.prototype.catch as well because Promise.prototype.then includes the 'catch' behavior, so the catch method is just another way of calling then. finally is essentially different.
There was a problem hiding this comment.
To clarify, you mean that if the subclass overrides then, it must then override finally as well, is that correct? Because a subclass doesn't have to override then unless it chooses to.
There was a problem hiding this comment.
To be fair, no one likes promise subclassing anyway :D
There was a problem hiding this comment.
The general consensus in the room - or at least, the only way to achieve consensus, which is effectively the same - was that either a) .catch and .finally should both directly delegate to .then, or, b) .catch and .finally should both call abstract operations directly. "b" is not web-compatible.
I'm not going to enumerate which committee members were blockers there, both to not throw folks under the bus and because it's irrelevant (the notes are not transcripts, intentionally) - I hope you can simply trust that what I've said above is the case.
There was a problem hiding this comment.
Can we phrase it so that engines have to implement finally through then only for non-subclasses? That would be sufficient - subclassing is almost never used anyway since async functions return the base class always anyway.
There was a problem hiding this comment.
Engines can already optimize out the .then calls when it's not a subclass and when Promise.prototype.then is unmodified; but no, if I replace Promise.prototype.then it needs to be able to observe the calls too.
There was a problem hiding this comment.
@ljharb I don't doubt what you're saying but I also can't help notice that the motivation for why those 2 options were the only ones considered by TC39 is missing. If TC39 isn't willing to evaluate (or re-evaluate, in case they already have) the aspects I've mentioned, this PR is DOA.
There was a problem hiding this comment.
The third option is what this PR does, which is what the finally proposal did originally, and what the committee rejected - see tc39/proposal-promise-finally#34
ljharb
added
normative change
|
I created the PR here because doing it in the other repo would have been much more difficult for me, as it differs a lot from this repo. (I'm not familiar with all the 'emu' stuff.) |
@ljharb When I submitted a similar PR to the proposal repository (exactly what you're asking @raulsebastianmihaila to do here), you told me I should submit it to this repository and/or your polyfill repo instead. What gives? I'm willing to believe there's a meaningful difference between these PRs that calls for submitting/reviewing them differently, but I feel the need to flag this apparent inconsistency in your preferences, and call out the possibility that you may be (consciously or unconsciously) derailing efforts to improve |
|
@raulsebastianmihaila the “emu” stuff is identical to this spec.html file :-) they’re both ecmarkup (not html) no worries tho. @benjamn sorry i was unclear; i asked to see what the spec diff would look like, but I’d intended you to update your PR in that repo (since that repo also has spec text and tests). I’m trying to avoid needless work; specifically, the work involved of convincing the committee that this change should happen, before the necessary legwork has been done (ie, showing that the changes are even feasible). |
|
Now that However, I think #1250 is likely to be a better and more generic approach, and I suspect that if that achieves consensus, this may be able to be closed. |
|
Closing, since #1250 has landed. |
https://esdiscuss.org/topic/promise-finally