Error when when tasks list contains non-functions

[JamesBarwell]

Hi, I am trying to use this module as a drop-in replacement for Promise.all in order to throttle some tasks. The docs say that it should behave the same way, however I've noticed that it behaves differently when passed already-resolved promises.

const Throttle = require('promise-parallel-throttle');

(async () => {
    function doThing() {
        return Promise.resolve('foo');
    };  

    const tasks = [ 
        doThing(),
    ];  

    //const result = await Promise.all(tasks); // returns [ 'foo' ]
    const result = await Throttle.all(tasks); // throws error

    console.log(result)
})();

You can comment and uncomment those lines to see the results.

Here's the error:

(node:17159) UnhandledPromiseRejectionWarning: Error: tasks[0]: [object Promise], is supposed to be of type function
    at executeTask (/home/james/dev/Promise-fail/node_modules/promise-parallel-throttle/build/throttle.js:73:31)
    at /home/james/dev/Promise-fail/node_modules/promise-parallel-throttle/build/throttle.js:97:21
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
    at Function.Module.runMain (module.js:695:11)
    at startup (bootstrap_node.js:191:16)
    at bootstrap_node.js:612:3
(node:17159) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:17159) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

node --version
v8.11.3
npm ls
/home/james/dev/Promise-fail
└── promise-parallel-throttle@3.1.0

It would be great if this behaviour could be fixed so that the module really could work a drop-in replacement. Cheers.

[DJWassink]

Outch you submitted this issue just when I went away on a holiday 😅

Thanks for the issue I will try to have a look at it somewhere in the following days.

[JamesBarwell]

When I submitted the ticket, as I say I was looking for a drop-in replacement for Promise.all, but afterwards I realised that that won't ever work because your module won't be able to throttle them if they've already been set off.

We had code like this:

const tasks = items.map(doAsyncThingWithItem)
return Promise.all(tasks)

I had to restructure it to this:

const taskCallbacks = items.map(item => doAsyncThingWithItem.bind(null, item))
return Throttle.all(taskCallbacks)

So given that it can't work as a drop-in replacement, I'm unsure what the desired behaviour should be. If you did add support then it wouldn't have broken, but it also wouldn't have done the throttling so it may have just failed silently.

I'm wondering if it might be simplest to update the documentation to remove the expectation that it can drop-in, or at least to call out that other code needs restructuring to allow your module to execute the tasks.

[DJWassink]

I was just looking for a fix and indeed came to the same conclusion. Maybe mentioning it in the readme is indeed a valid case. For now I will publish a little update (#26) which adds and extra option to ignore the error you mentioned from beign thrown. Instead it simply returns the current task.

[JamesBarwell]

Cheers.