Add finally method to Promises

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: nhnt11, Assigned: evilpie)

Tracking

(Blocks: 3 bugs, {dev-doc-complete})

Trunk
mozilla58
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
Promises already have a catch method (= promise.then(null, foo)), a finally method would be useful (= promise.then(foo, foo))
(Reporter)

Comment 1

5 years ago
Created attachment 8432661 [details] [diff] [review]
Patch

This patch adds Promise.prototype.finally, which calls then() with identical arguments.

Updated

5 years ago
Blocks: 885333
Component: Async Tooling → DOM
Product: Toolkit → Core
This is a spec-level issue, no?  I don't think we should implement this unless there's a good chance it will end up in the spec.
(Reporter)

Comment 3

5 years ago
I was looking at the Promises/A+ spec, just saw https://github.com/domenic/promises-unwrapping. There is indeed a closed issue about adding Promise.prototype.finally (which is not as trivial as the attached patch suggests by the way) - https://github.com/domenic/promises-unwrapping/issues/18. It was closed, however, so as to "not add new features that will delay the whole thing longer". I guess this bug should be closed for now then and possibly reopened if Promise.prototype.finally ever gets added in the spec.

Comment 4

5 years ago
If we think there is a need for such a method then it should be presented as a proposal to TC39 before initially implement it in FF.  

See me or dherman about how to proceed down that pat.

Comment 5

5 years ago
(In reply to allenwb from comment #4)
> If we think there is a need for such a method then it should be presented as
> a proposal to TC39 before initially implement it in FF.  
> 
> See me or dherman about how to proceed down that pat.

err, path

Comment 6

5 years ago
I am happy to champion `Promise.prototype.finally` for ES7.
A spec now exists for this: https://github.com/tc39/proposal-promise-finally

It's at stage 2 right now, meaning we could implement but not let it ride the trains. Needinfo myself to not forget it.
Component: DOM → JavaScript: Standard Library
Flags: needinfo?(till)
OS: Mac OS X → Unspecified
Hardware: x86 → Unspecified
Duplicate of this bug: 1384505
Flags: needinfo?(till)
Keywords: dev-doc-needed
Till, does the attached patch still apply?
Flags: needinfo?(till)

Comment 12

2 years ago
This is now at stage 3 - can this patch get a train ticket? :-D
(In reply to Boris Zbarsky [:bz] from comment #11)
> Till, does the attached patch still apply?

Even if it does, it's too outdated for the current Promise.prototype.finally proposal. :-)
Flags: needinfo?(till)
Created attachment 8892643 [details] [diff] [review]
bug1019116.patch

Untested and still contains a few TODOs and FIXMEs.
Attachment #8432661 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
Chrome 61 added this under a flag.
(Assignee)

Comment 16

a year ago
Do you want to finish this?
Flags: needinfo?(andrebargull)
(In reply to Tom Schuster [:evilpie] from comment #16)
> Do you want to finish this?

No, not right now.
Flags: needinfo?(andrebargull)
Chrome 63 and Safari TP already support this, so I believe the current spec has a good chance to became stable.

Comment 19

a year ago
It's stage 3; the spec is already stable. I'm going to seek stage 4 next month. It would be great if Firefox had a shipping implementation as well.
(Assignee)

Comment 20

a year ago
I just submitted the two issues point out by André on github repo. I will try to get this updated after those are resolved.
(Assignee)

Updated

a year ago
Blocks: 652780
(Assignee)

Updated

a year ago
Assignee: nobody → evilpies
(Assignee)

Comment 21

a year ago
Created attachment 8925334 [details] [diff] [review]
Remove old Promise selfhosting functions

I guess after we stopped self-hosting most of the promise code those functions aren't used anymore. I am mostly trying to prevent the confusion between ResolvePromise and PromiseResolve.
Attachment #8925334 - Flags: review?(till)
(Assignee)

Comment 22

a year ago
Created attachment 8925335 [details] [diff] [review]
Add Promise.prototype.finally

I just refreshed the patch and removed a bunch of FIXME/TODO that have been resolved. No functional changes.
Attachment #8892643 - Attachment is obsolete: true
Attachment #8925335 - Flags: review?(till)
(Assignee)

Comment 23

a year ago
Created attachment 8925336 [details] [diff] [review]
Import Promise.prototype.finally test262 tests

I re-imported the last import with `python test262-update.py --revision a456b0a390bb0f70b4cb8d38cb5ab0ecb557a851`, but accepting Promise-prototype-finally tests. The next full import is going to add two more tests, that we also pass.
Attachment #8925336 - Flags: review?(andrebargull)
Attachment #8925336 - Flags: review?(andrebargull) → review+
Comment on attachment 8925335 [details] [diff] [review]
Add Promise.prototype.finally

Review of attachment 8925335 [details] [diff] [review]:
-----------------------------------------------------------------

ResolvePromise isn't entirely correct. The fix should, if anything, simplify things, but I'm somewhat surprised tests didn't catch this, so I might be missing something. Clearing review for now.

::: js/src/builtin/Promise.cpp
@@ +2276,5 @@
> +js::PromiseResolve(JSContext* cx, HandleObject constructor, HandleValue value)
> +{
> +    // Step 1 (implicit).
> +
> +    // Step 2.

Step 2 of PromiseResolve does an IsPromise check, and it needs to do that to work for Promise.resolve. I'm surprised no test262 tests caught this as it changes the timing of callback invocation.

AFAICT, you should be able to just use CommonStaticResolveRejectImpl for this.

::: js/src/builtin/Promise.js
@@ +30,5 @@
> +        thenFinally = onFinally;
> +        catchFinally = onFinally;
> +    } else {
> +        // ThenFinally Function.
> +        (thenFinally) = function(value) {

Why the () around thenFinally? I see it was already contained in anba's patch, but it's not at all clear to me why it'd be needed. If I'm not missing something, please remove it.

@@ +50,5 @@
> +            return callContentFunction(promise.then, promise, function() { return value; });
> +        };
> +
> +        // CatchFinally Function.
> +        (catchFinally) = function(reason) {

Same here.
Attachment #8925335 - Flags: review?(till)
(In reply to Till Schneidereit [:till] from comment #24)
> ::: js/src/builtin/Promise.js
> @@ +30,5 @@
> > +        thenFinally = onFinally;
> > +        catchFinally = onFinally;
> > +    } else {
> > +        // ThenFinally Function.
> > +        (thenFinally) = function(value) {
> 
> Why the () around thenFinally? I see it was already contained in anba's
> patch, but it's not at all clear to me why it'd be needed. If I'm not
> missing something, please remove it.

That's just a silly, but spec-conformant(!), trick to disable function name inferring. :-)

js> foo = function(){}; foo.name
"foo"
js> (foo) = function(){}; foo.name
""
(In reply to André Bargull [:anba] from comment #25)
> That's just a silly, but spec-conformant(!), trick to disable function name
> inferring. :-)

Ooh, right! :) Thanks for the explanation.
(Assignee)

Comment 27

a year ago
Created attachment 8925538 [details] [diff] [review]
v2 Add Promise.prototype.finally
Attachment #8925538 - Flags: review?(till)
(Assignee)

Updated

a year ago
Attachment #8925335 - Attachment is obsolete: true
> That's just a silly, but spec-conformant(!), trick to disable function name inferring. :-)

This is the sort of thing that needs an in-code comment so someone won't "fix" it later.
Comment on attachment 8925538 [details] [diff] [review]
v2 Add Promise.prototype.finally

Review of attachment 8925538 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, thanks!
Attachment #8925538 - Flags: review?(till) → review+
(In reply to André Bargull [:anba] from comment #25)
> That's just a silly, but spec-conformant(!), trick to disable function name
> inferring. :-)

you monster
(Just remarking on that being nutso like the stupid hacks depended on to disable syntax parsing, not criticizing its use if actually demanded by spec.  If.  :-) )
Comment on attachment 8925334 [details] [diff] [review]
Remove old Promise selfhosting functions

Review of attachment 8925334 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8925334 - Flags: review?(till) → review+

Comment 33

a year ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca31c599c21
Remove old Promise selfhosting functions. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab4309be28d
Implement Promise.prototype.finally. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc05d670729c
Import Promise.prototype.finally test262 tests. r=anba
(Assignee)

Updated

a year ago
Keywords: leave-open
(Assignee)

Comment 34

a year ago
Created attachment 8926022 [details] [diff] [review]
Remove brand check

This isn't even tested by the tests, but we shouldn't land something that is already incorrect.

I am going to see to get us updated to a newer test262 version and some tests added for that.
Attachment #8926022 - Flags: review?(andrebargull)
Backed out for failing tc-M(c3) in js/xpconnect/tests/chrome/test_xrayToJS.xul:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc777e5019993d68de03a83d5b8cfe54fece31d8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fc05d670729c3d7632e8783a040ec9df22ae56b4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=142776025&repo=mozilla-inbound

[task 2017-11-07T17:55:57.844Z] 17:55:57     INFO - TEST-PASS | js/xpconnect/tests/chrome/test_xrayToJS.xul | Xray global is correct 
[task 2017-11-07T17:55:57.844Z] 17:55:57     INFO - TEST-PASS | js/xpconnect/tests/chrome/test_xrayToJS.xul | Underlying global is correct 
[task 2017-11-07T17:55:57.845Z] 17:55:57     INFO - TEST-PASS | js/xpconnect/tests/chrome/test_xrayToJS.xul | Results match 
[task 2017-11-07T17:55:57.846Z] 17:55:57     INFO - TEST-PASS | js/xpconnect/tests/chrome/test_xrayToJS.xul | Unmodified flags accessors are called 
[task 2017-11-07T17:55:57.846Z] 17:55:57     INFO - TEST-PASS | js/xpconnect/tests/chrome/test_xrayToJS.xul | Unmodified flags and source accessors are called 
[task 2017-11-07T17:55:57.847Z] 17:55:57     INFO - TEST-PASS | js/xpconnect/tests/chrome/test_xrayToJS.xul | Unmodified flags and source accessors are called 
[task 2017-11-07T17:55:57.848Z] 17:55:57     INFO - Buffered messages finished
[task 2017-11-07T17:55:57.868Z] 17:55:57     INFO - TEST-UNEXPECTED-FAIL | js/xpconnect/tests/chrome/test_xrayToJS.xul | A property on the Promise prototype has changed! You need a security audit from an XPConnect peer - got "[\"catch\", \"constructor\", \"finally\", \"then\"]", expected "[\"catch\", \"constructor\", \"then\"]"
[task 2017-11-07T17:55:57.868Z] 17:55:57     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2017-11-07T17:55:57.868Z] 17:55:57     INFO - testXray@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xul:404:5
[task 2017-11-07T17:55:57.868Z] 17:55:57     INFO - testPromise@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xul:906:5
[task 2017-11-07T17:55:57.869Z] 17:55:57     INFO - go@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xul:146:5
[task 2017-11-07T17:55:57.869Z] 17:55:57     INFO - onload@chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_xrayToJS.xul:1:1
[task 2017-11-07T17:55:57.869Z] 17:55:57     INFO - TEST-PASS | js/xpconnect/tests/chrome/test_xrayToJS.xul | A symbol-keyed property on the Promise prototype has been changed! You need a security audit from an XPConnect peer
Flags: needinfo?(evilpies)
Comment on attachment 8926022 [details] [diff] [review]
Remove brand check

Review of attachment 8926022 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8926022 - Flags: review?(andrebargull) → review+
(Assignee)

Comment 37

a year ago
Created attachment 8926051 [details] [diff] [review]
v3 Add Promise.prototype.finally

Folded the brand check removal into this patch. Added "finally" to test_XrayToJS
Attachment #8925538 - Attachment is obsolete: true
Attachment #8926022 - Attachment is obsolete: true
Flags: needinfo?(evilpies)
(Assignee)

Updated

a year ago
Attachment #8926051 - Flags: review?(bzbarsky)
Comment on attachment 8926051 [details] [diff] [review]
v3 Add Promise.prototype.finally

r=me on the test_xrayToJS.xul bit

If you wanted me to review the rest of this, please let me know; I'm assuming someone else is doing that!
Attachment #8926051 - Flags: review?(bzbarsky) → review+

Comment 39

a year ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/919528a717da
Remove old Promise selfhosting functions. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/af50babd9968
Implement Promise.prototype.finally. r=till,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/2af1c7c2375f
Import Promise.prototype.finally test262 tests. r=anba
(Assignee)

Updated

a year ago
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.