blend / promise-utils

Lodash-like, dependency-free utilities for native ES6 promises.
MIT License
282 stars 15 forks source link

map does not work with iterables other than strings, arrays and plain objects #52

Open gurpreetatwal opened 4 years ago

gurpreetatwal commented 4 years ago

The JSDoc on map indicates that input can be an iterable, however it doesn't seem to work with other objects that implement the iterable protocol including built-in ones like Set.

Test Case

const P = require('blend-promise-utils');

const main = async function () {
  const numbers = new Set([1, 2, 3]);
  await P.map(numbers, function (number) {
    console.log(number);
  });
};

main();
Expected output
1
2
3
Actual output
gurpreetatwal commented 4 years ago

Two potential resolutions:

  1. Edit the JSDoc to remove iterable and replace it with string
  2. Edit the code to add support for iterable

My vote is for resolution two for the following reasons:

  1. personal bias :stuck_out_tongue_winking_eye: -- modifying map to support all Iterables fixes my problem
  2. users might assume that the function supports iterables and the behavior as implemented causes the code to fail silently leading to errors that are hard to debug
  3. supporting Iterables "feels" correct (which is most likely why the JSDoc calls out)

More than happy to help out with a PR for either of those or any other solutions :)

ftrimble commented 4 years ago

Do you have an idea for maintaining the same input typing for the output as the input if we support generic iterables? Generally I don't see an easy way to achieve that, which is why I'd probably rather update the docs.

gurpreetatwal commented 4 years ago

By "input typing for the output" do you mean the return value for the map function itself or for the input into the iteratee function? Just want to make sure we're looking at the same thing

ftrimble commented 4 years ago

i.e. the return type for map is the same as the input type