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

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

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.

+1

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.

There is new bug here https://bugzilla.mozilla.org/show_bug.cgi?id=1384505

Till, does the attached patch still apply?

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. :-)

Untested and still contains a few TODOs and FIXMEs.

Chrome 61 added this under a flag.

Do you want to finish this?

(In reply to Tom Schuster [:evilpie] from comment #16) > Do you want to finish this? No, not right now.

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.

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.

I just refreshed the patch and removed a bunch of FIXME/TODO that have been resolved. No functional changes.

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.

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.

(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.

> 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!

(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

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

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.

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

Comment on attachment 8926022 [details] [diff] [review] Remove brand check Review of attachment 8926022 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Folded the brand check removal into this patch. Added "finally" to test_XrayToJS

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!

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

https://hg.mozilla.org/mozilla-central/rev/919528a717da https://hg.mozilla.org/mozilla-central/rev/af50babd9968 https://hg.mozilla.org/mozilla-central/rev/2af1c7c2375f

This got documented: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally Browser compat data: https://github.com/mdn/browser-compat-data/pull/663