feat: add rejectLate option

[wraithgar]

Closes: #1

[wraithgar]

I should also update the README, which I'll do if I get the indication this is gonna land.

[isaacs]

This seems surprising. If you have rejectLate set, then shouldn't it process all items, even if there's an early rejection? It looks like it's not actually doing that.

const promiseCallLimit = require('./')

const pass = async () => new Promise(r => setTimeout(() => {
  console.log('pass')
  r()
}))
const fail = async () => new Promise((_, r) => setTimeout(() => {
  console.log('fail')
  r('nope')
}))

promiseCallLimit([
  pass,
  fail,
  pass,
  pass,
  pass,
  pass,
  pass,
], 2, true).then(console.log, console.error)

// Expect 6 passes logged, 1 fail
// Actual:
// pass
// fail
// pass
// nope

[isaacs]

Or is rejectLate only intended to reject after all started items, not after all items in the list?

[wraithgar]

Yes, the idea is that if any in-flight promise rejects, no new ones are started, but all in-flight promises finish before that first rejection bubbles up. Any extra rejections are disregarded.

[isaacs]

Consider:

diff --git a/index.js b/index.js
index afba90d..e6a8a63 100644
--- a/index.js
+++ b/index.js
@@ -36,20 +36,20 @@ const callLimit = (queue, limit = defLimit, rejectLate) => new Promise((res, rej
   const run = () => {
     const c = current++
     if (c >= queue.length) {
-      return resolve()
+      return rejected ? reject() : resolve()
     }
 
     active ++
     results[c] = queue[c]().then(result => {
       active --
       results[c] = result
-      if (!rejected)
+      if (!rejected || rejectLate)
         run()
       return result
     }, (er) => {
       active --
       reject(er)
-    }).finally(result => {
+    }).then(result => {
       if (rejectLate && rejected && active === 0)
         return rej(rejection)
       return result

[isaacs]

Yes, the idea is that if any in-flight promise rejects, no new ones are started, but all in-flight promises finish before that first rejection bubbles up. Any extra rejections are disregarded.

I see. That seems fine, and is what this does, but should definitely be documented, because it's easy to assume that rejectLate will behave like promise-all-reject-late, where all promises are guaranteed to resolve before returning.

[isaacs]

Maybe instead of a rejectLate boolean, it should be an options object? I'm not sure how to best capture the different senses of "rejectLate" behavior, but they both seem reasonable to want, in different scenarios.

[wraithgar]

Consider:

This makes the t.notOk(failure2, 'slow rejection never ran') test on line 88 fail.

The existing tests also never catch the entirety of if (!rejected || rejectLate)

ETA: Oh does this run all promises? That'd be a good implementation of "reject last" as I'm calling it.

[wraithgar]

I'd love to switch to an options object. I opted for a non-breaking change for the initial PR.

[wraithgar]

As far as the "reject late" vs "reject last" distinction, that isn't something npm itself needs but I can see folks eventually wanting it.

[wraithgar]

Since we're in net new territory if folks want ALL the promises to run they really want a Promise.allSettled behavior and perhaps that would be the option name.

[wraithgar]

Took a stab at an opts object and updating the readme.

[isaacs]

ETA: Oh does this run all promises? That'd be a good implementation of "reject last" as I'm calling it.

Yeah, that's when I thought the intent was for rejectLate to run all the way to the end, we had a few rapid-fire comments pass midair there, I think 😅

[wraithgar]

It's entirely possible my initial assumption about how rejectLate would work is wrong. I had assumed the behavior in this PR would be the sensible default but I can now see how rejectLate already has a pretty tight coupling to the allSettled behavior.

I also assumed this was the behavior we'd want in npm, but it's very likely I was wrong.

Ignoring my assumption so that we can try to get rid of the scope creep here, what do you think the minimum implementation of this should be here?

[wraithgar]

Rewrote the PR to discard the idea of stopping at the first rejection.

[wraithgar]

Breaking change