Re: Intent to ship: Promise.allSettled

2019-11-02 Thread Paolo Amadini

On 11/1/2019 2:42 PM, Jan-Ivar Bruaroey wrote:
My original point was that the semantics of Promise.allSettled, which 
are "keep waiting for the lot even if some async operations fail", did 
not deserve its own standard name in the JS language, because of


A) how rarely this is actually what you want,
B) how easy it is to accomplish when it is, using patterns like e.g.
    Promise.all(promises.map(p => p.catch(e => e)) & friends, and
C) (your point?) bugs from people wrongly using instead of Promise.all


Yeah, my own reasoning was more strictly scoped to testing and the
mozilla-central repository because that is my area of expertise, and I
may miss reasons why allSettled could be more useful in JavaScript in
other environments, but your general points look valid to me as well.

Cheers,
Paolo
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Consider avoiding allSettled in tests (was: Re: Intent to ship: Promise.allSettled)

2019-11-01 Thread Kris Maglione
On Thu, Oct 31, 2019, 06:02 Jason Orendorff  wrote:

> Ignoring the awaited value here is like using `catch {}` to squelch all
> exceptions, or ignoring the return value of an async function or method, or
> any other expression that produces a Promise. Do we have lints for those
> pitfalls? I'm kind of surprised that there doesn't seem to be a built-in
> ESLint lint for `catch {}`.
>

Some directories have an ESLint rule to disallow empty blocks, which
requires that any empty catch blocks at least contain a comment to justify
them.

>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Intent to ship: Promise.allSettled

2019-11-01 Thread Jan-Ivar Bruaroey

On 11/1/19 6:08 AM, Paolo Amadini wrote:

On 10/31/2019 1:57 PM, Jan-Ivar Bruaroey wrote:
The context here is the same as Promise.allSettled: we explicitly *do* 
want to ignore errors, right?


In general, in mozilla-central we want to at least log those errors, at
which point they are already caught, and using "Promise.all" is then
equivalent (which if I understand correctly was your original point).


No, they're never equivalent, except where failure is impossible. 
Promise composition tools are different waiting semantics. [1]


We're discussing the JS language here, not testing & mozilla-central.

My original point was that the semantics of Promise.allSettled, which 
are "keep waiting for the lot even if some async operations fail", did 
not deserve its own standard name in the JS language, because of


A) how rarely this is actually what you want,
B) how easy it is to accomplish when it is, using patterns like e.g.
   Promise.all(promises.map(p => p.catch(e => e)) & friends, and
C) (your point?) bugs from people wrongly using instead of Promise.all

.: Jan-Ivar :.

[1] 
https://github.com/tc39/proposal-promise-any/#ecmascript-proposal-promiseany


.: Jan-Ivar :.



Cheers,
Paolo

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Intent to ship: Promise.allSettled

2019-10-31 Thread Jan-Ivar Bruaroey

On 10/31/19 4:26 AM, Paolo Amadini wrote:

On 10/30/2019 10:19 PM, Jan-Ivar Bruaroey wrote:

 Promise.all(promises.map(p => p.catch(e => e)))
https://stackoverflow.com/questions/31424561/wait-until-all-promises-complete-even-if-some-rejected/36115549#36115549 



By the way, this "catch(e => e)" call may make sense as an example in
the Stack Overflow answer (and thanks for posting it here), I just want
to point out that taken out of context it is clearly an anti-pattern.


The context here is the same as Promise.allSettled: we explicitly *do* 
want to ignore errors, right?


I thought you had a great point against Promise.allSettled: it competes 
with Promise.all for attention among people who don't understand the 
difference, leading them to use a flame thrower instead of garden rake.


At least in my pattern, there's a `catch` prominently displayed giving a 
clue errors are willfully being captured.



Unless you have guarantees about the value of "e" and the expectations
around the return value of "Promise.all", muddying the rejection and
fulfillment values together may lead to bugs that occur rarely and are
difficult to track, and potentially even security bugs.


Yes, Boris gives a proper polyfill. I'd counter that unless you're 
writing some generic library, in almost any other context, the 
anti-pattern to me is *not* having guarantees (or at least expectations) 
around the types of r and e. That said, everything you say is true.



Rejection handling has been one of the most common mistakes I've seen
when reviewing patches (although that was a while ago when promises were
still new), so it's good to be aware of those anti-patterns.


No disagreement there. Hopefully the awkward and longer name will deter 
all but those who really need it from reaching for allSettled. But yeah, 
I'm in the camp that we shouldn't have added it.


Cheers!

.: Jan-Ivar :.



Cheers,
Paolo


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Consider avoiding allSettled in tests (was: Re: Intent to ship: Promise.allSettled)

2019-10-31 Thread Jason Orendorff
On Thu, Oct 31, 2019 at 4:10 AM Paolo Amadini 
wrote:

>// INCORRECT
>//await Promise.allSettled([promise1, promise2]);
>
> The last example silently loses any rejection state.
>

Ignoring the awaited value here is like using `catch {}` to squelch all
exceptions, or ignoring the return value of an async function or method, or
any other expression that produces a Promise. Do we have lints for those
pitfalls? I'm kind of surprised that there doesn't seem to be a built-in
ESLint lint for `catch {}`.

There's nothing special about tests here, right? Those patterns are fishy
whether you're in a test or not.

-j
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Intent to ship: Promise.allSettled

2019-10-31 Thread Paolo Amadini

On 10/30/2019 10:19 PM, Jan-Ivar Bruaroey wrote:

     Promise.all(promises.map(p => p.catch(e => e)))
https://stackoverflow.com/questions/31424561/wait-until-all-promises-complete-even-if-some-rejected/36115549#36115549 


By the way, this "catch(e => e)" call may make sense as an example in
the Stack Overflow answer (and thanks for posting it here), I just want
to point out that taken out of context it is clearly an anti-pattern.

Unless you have guarantees about the value of "e" and the expectations
around the return value of "Promise.all", muddying the rejection and
fulfillment values together may lead to bugs that occur rarely and are
difficult to track, and potentially even security bugs.

Rejection handling has been one of the most common mistakes I've seen
when reviewing patches (although that was a while ago when promises were
still new), so it's good to be aware of those anti-patterns.

Cheers,
Paolo
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Consider avoiding allSettled in tests (was: Re: Intent to ship: Promise.allSettled)

2019-10-31 Thread Paolo Amadini

On 10/30/2019 10:19 PM, Jan-Ivar Bruaroey wrote:

This always seemed trivial to me to do with:
     Promise.all(promises.map(p => p.catch(e => e)))


As Boris pointed out, this does not have proper exception handling. If
exceptions should be ignored, it may be good to call "console.error". In
case exceptions should be fatal, like in most automated test files in
mozilla-central, something like the following would be better instead:

  .catch(ex => ok(false, ex))

But unless you have an array to begin with, in test files it's generally
better to "await" on each promise. There is a danger with allSettled:

  // Better
  await promise1;
  await promise2;

  // Also valid
  await Promise.all([promise1, promise2]);

  // INCORRECT
  //await Promise.allSettled([promise1, promise2]);

The last example silently loses any rejection state.

My recommendation to avoid this pitfall would be to add an ESLint rule.
However, I don't know how sophisticated they could be, and just checking
that the caller does something with the return value may not be enough,
if passing it to "await" or calling ".then" is enough to silence the
warning. Maybe just disallow allSettled in tests unless overridden?

Cheers,
Paolo
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Intent to ship: Promise.allSettled

2019-10-30 Thread Jan-Ivar Bruaroey

Promise.any is an inverse of Promise.all, right? There and back again:

Promise.all(promises.map(p => p.then(r => Promise.reject(r), e => e)))
  .then(r => Promise.reject(r), e => e)

https://jsfiddle.net/jib1/f085bcyk/

That said, Promise.any has a nice ring to it.

.: Jan-Ivar :.

On 10/30/19 6:39 PM, Jason Orendorff wrote:

Rather; but there is still a (rapidly closing) window to comment on the
*next* proposal in this vein: Promise.all <
https://github.com/tc39/proposal-promise-any/#ecmascript-proposal-promiseany>.
It is a stage 3 proposal now. Our representative on the ECMAScript standard
committee is Yulia Startsev (or you can just email me).

-j



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Intent to ship: Promise.allSettled

2019-10-30 Thread Jan-Ivar Bruaroey
Yes, it would have been clever of me to predict the semantics 3 years 
ago. ;-)


But as I mention in the stackoverflow post, depending on context and the 
type(s) of values expected, errors can often be distinguished easily 
enough in practice, if not in general.


That said, I've never actually had an opportunity to use this pattern 
for anything...


.: Jan-Ivar :.

On 10/30/19 6:39 PM, Boris Zbarsky wrote:

On 10/30/19 6:19 PM, Jan-Ivar Bruaroey wrote:

This always seemed trivial to me to do with:

 Promise.all(promises.map(p => p.catch(e => e)))


This has different semantics from Promise.allSettled, right?  In 
particular, it effectively treats rejected promises as resolved with the 
rejection value, forgetting the fact that they were rejected. 
Promise.allSettled, on the other hand, preserves information about the 
state.


I think you can still polyfill it, using something like:

Promise.all(promises.map(p => p.then(
   v => { { status: "fulfilled", value: v } },
   e => { { status: "rejected", reason: e } }));

though I think this might create more temporary Promises than allSettled 
does...


-Boris


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Intent to ship: Promise.allSettled

2019-10-30 Thread Boris Zbarsky

On 10/30/19 6:19 PM, Jan-Ivar Bruaroey wrote:

This always seemed trivial to me to do with:

     Promise.all(promises.map(p => p.catch(e => e)))


This has different semantics from Promise.allSettled, right?  In 
particular, it effectively treats rejected promises as resolved with the 
rejection value, forgetting the fact that they were rejected. 
Promise.allSettled, on the other hand, preserves information about the 
state.


I think you can still polyfill it, using something like:

Promise.all(promises.map(p => p.then(
  v => { { status: "fulfilled", value: v } },
  e => { { status: "rejected", reason: e } }));

though I think this might create more temporary Promises than allSettled 
does...


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: Intent to ship: Promise.allSettled

2019-10-30 Thread Jan-Ivar Bruaroey

This always seemed trivial to me to do with:

Promise.all(promises.map(p => p.catch(e => e)))

https://stackoverflow.com/questions/31424561/wait-until-all-promises-complete-even-if-some-rejected/36115549#36115549

But I guess it's too late to make that point. I guess the more 
primitives the merrier!


.: Jan-Ivar :.

On 10/30/19 5:44 PM, Jason Orendorff wrote:

In Firefox 71, we'll ship Promise.allSettled, a standard way to `await`
several promises at once. André Bargull [:anba] contributed the
implementation of this feature. It's in Nightly now.

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1539694
Shipped in: https://bugzilla.mozilla.org/show_bug.cgi?id=1549176

Standard: https://tc39.es/ecma262/#sec-promise.allsettled

MDN:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/allSettled

Platform coverage: All, no pref

DevTools bug: N/A. The DevTools don't currently have any custom support
for peeking at the internal state of Promise objects.

Other browsers: Shipped in Chrome 76, Safari 13.

Testing: There are test262 tests covering this feature:
https://github.com/tc39/test262/tree/master/test/built-ins/Promise/allSettled

Use cases: Promise.allSettled is useful in async code. It's used to wait
for several tasks to finish in parallel. What sets it apart from the
existing methods Promise.race and Promise.all is that it *doesn't*
short-circuit as soon as a single task succeeds/fails.

Secure contexts: This is a JS language feature and is therefore present
in all contexts.

-j



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform