Closed Bug 1019116 Opened 10 years ago Closed 6 years ago

Add finally method to Promises

Categories

(Core :: JavaScript: Standard Library, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: nhnt11, Assigned: evilpie)

References

(Blocks 3 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 5 obsolete files)

Promises already have a catch method (= promise.then(null, foo)), a finally method would be useful (= promise.then(foo, foo))
Attached patch Patch (obsolete) — Splinter Review
This patch adds Promise.prototype.finally, which calls then() with identical arguments.
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.
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.
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.
(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
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
Flags: needinfo?(till)
Keywords: dev-doc-needed
Till, does the attached patch still apply?
Flags: needinfo?(till)
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)
Attached patch bug1019116.patch (obsolete) — Splinter Review
Untested and still contains a few TODOs and FIXMEs.
Attachment #8432661 - Attachment is obsolete: true
Chrome 61 added this under a flag.
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.
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.
I just submitted the two issues point out by André on github repo. I will try to get this updated after those are resolved.
Blocks: test262
Assignee: nobody → evilpies
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)
Attached patch Add Promise.prototype.finally (obsolete) — Splinter Review
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)
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.
Attached patch v2 Add Promise.prototype.finally (obsolete) — Splinter Review
Attachment #8925538 - Flags: review?(till)
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+
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
Keywords: leave-open
Attached patch Remove brand check (obsolete) — Splinter Review
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+
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)
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+
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
Keywords: leave-open
Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.