Add support for AsyncIterable as input

[huntharo]

Overview

• Expand capabilities to handle both async and sync iterables - Fixes #20

Open Issues

• [x] Duplicating the tests for the async versions doesn't feel right - Let me know if there is a better way to handle this with ava
• [x] There is a problem in the original functionality around the stop-on-error test that needs to be resolved
  • The test has a bug in that neither p-map nor the test mapper function, which is provided for that specific test and is not the common version, await the calling of the delay() function for item 2
  • This means that item 2 will only ever return a Promise, never a value
  • When this is fixed to await the function call, it causes the returned results to change from [1, 3] to [1, 3, 2]
  • Fixing this test makes it clear that the concurrency setup loop does not wait for mappers, so the only way to make this test fail would be to have enough items that eventually the initial iteration could not reach the last items in the test array before the 100 ms delay on the exception expires
  • With concurrency = 1 this test behaves as expected since there are not unlimited unawaited promises created
  • Let me know your thoughts on how to resolve this as it becomes more apparent with the behavior changes needed for asyncIterable (where the concurrency setup iteration of asyncIterable has to be awaited to prevent inifinte runners from being created)
  • The test, in the state it's in in this PR, does not actually demonstrate that stop on error works as intended

[sindresorhus]

Duplicating the tests for the async versions doesn't feel right

That's how it's done. There's good way of mixing async and sync in JS.

[sindresorhus]

There is a problem in the original functionality around the stop-on-error test that needs to be resolved

Which test exactly? Can you link to it?

[huntharo]

There is a problem in the original functionality around the stop-on-error test that needs to be resolved

Which test exactly? Can you link to it?

This test: https://github.com/sindresorhus/p-map/blob/main/test.js#L121-L135

Stop on error is problematic... there can be many concurrent mappers running and they will continue to run even if iteration is "stopped" after one of them throws. If the continuing mappers also throw they will have nowhere to throw to that can be caught. So it seems that the best way to handle an exception in one mapper is to check if any other mappers are running, push the exception onto an array, then let the other mappers either succeed or fail, and the last of them throws and AggregateError with the exceptions from all of them or just throws the single error if only one of them failed. It definitely should not throw until all of the mappers have finished though as that could lead to the consumer restarting the pMap while mappers from the prior run are still active.

Ultimately what I ended up thinking was: if you need clean error handling then you should never allow the mapper to throw and instead handle the error yourself in the mapper. However, I don't think there is a way for the mapper to signal that iteration should stop early.

Current form of the test in the PR

• The changes here are:
  • Add an async closure for the delay call so it can be detected as a function to be awaited - Checking instanceof Promise or ClearablePromise didn't work in this case
  • When the input value is a function, await the call of the function
  • Change the expected return value to 1, 3, 2 - Which is "wrong" as it shows that all values up to concurrency get mapped even if one of them throws an error before the others finish
• The test does not demonstrate that iteration / mapper invocation stops after a mapper throws because the default concurrency is infinite and all of the mappers will be invoked before any one of them throws an exception - We can change this to have concurrency 1 and it will then return a result of [1], but all 3 mappers will still be invoked and will run to completion.

test('do not run mapping after stop-on-error happened', async t => {
    const input = [1, async () => delay(300, {value: 2}), 3];
    const mappedValues = [];
    await t.throwsAsync(
        pMap(input, async value => {
            value = typeof value === 'function' ? await value() : value;
            mappedValues.push(value);
            if (value === 1) {
                await delay(100);
                throw new Error('Oops!');
            }
        })
    );
    await delay(500);
    t.deepEqual(mappedValues, [1, 3, 2]);
});

[sindresorhus]

This test: https://github.com/sindresorhus/p-map/blob/main/test.js#L121-L135

// @papb ^

[huntharo]

This test: https://github.com/sindresorhus/p-map/blob/main/test.js#L121-L135

// @papb ^

I added a PR with a failing test case that shows that multiple concurrent mapper exceptions result in only one of those errors being catchable:

49

I think the stopOnError true/false logic can be somewhat consolidated so that both reject with an AggregateError when mappers have all stopped, but stopOnError true immediately stops invoking more mappers and iterating more source items (if applicable, as in concurrency !== Infinity).

[sindresorhus]

I think the stopOnError true/false logic can be somewhat consolidated so that both reject with an AggregateError when mappers have all stopped

Per https://github.com/sindresorhus/p-map/pull/49#issuecomment-916271810, stopOnError: true should only return a single error. This is how Promise.all works too.

[huntharo]

I believe all outstanding issues have been addressed now.

[sindresorhus]

Can you update the readme too?

[huntharo]

Can you update the readme too?

The readme has been updated.

[sindresorhus]

Looks good. Thanks for contributing this :)

[huntharo]

Looks good. Thanks for contributing this :)

No problem. Thanks for this (and other) modules and for the timely reviews and merges!