New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Treat functions without a callback as promises #1673
Comments
We have no way of being able to detect reliably whether a function returns a promise or not, and whether we need to pass a callback to it or not beforehand. ( |
@aearly hmm right, so maybe something simpler like detecting the callback at the top-level? async function parallel(o, cb) {
if (!cb) { // promise behavior
return Object.fromEntries(
await Promise.all(Object.entries(o).map(async ([k, v]) => [k, await v()]))
)
}
// else do the callback behavior
} |
We already so something like that -- return a promise if you omit the callback to |
Ok, let's be concrete: https://repl.it/@caub/async-async // npm.im/async expects all inner functions to be async,
// even when using no outer callback (as 2nd arg of async.parallel)
console.log(
await async.parallel({
foo: async () => delay(1000).then(() => 1),
bar: async (rs) => {
return 2
},
})
);
// Doesn't work without those async,
// but I'd like this to resolve just like above instead of freezing
console.log(
await async.parallel({
foo: () => delay(1000).then(() => 1),
bar: () => {
return 2
},
})
); |
I think, in a future next major version, it could be interesting to only support callback in a way similar to this: await Promise.all([
{ then(resolve){ setTimeout(resolve, 50, 1) } }, // callback way
2,
delay(45).then(())=>3)
])
// [1, 2, 3] removing the need for unnecessary The advantage is more clarity. I understand your point about not assuming user intent, but you're still assuming people will not forget the |
That would break backwards compatibility in a pretty huge way -- for example you couldn't do things like |
Could you explain where it would break? What currently break is: await async.map(['server.js', 'package.json'], fs.readFile) // works
await async.map(['server.js', 'package.json'], fs.promises.readFile) // works
await async.map(['server.js', 'package.json'], s => fs.promises.readFile(s)) // doesn't work
await async.map(['server.js', 'package.json'], async s => fs.promises.readFile(s)) // works The idea is to fix the 3rd one |
I get those problems constantly, for example I wrote: // ...
doc = await async.map(items, item => item.toDoc(currentUser, options)); somewhere, and again, timeout because I was missing a If I'm alone in that case, it's not worth, but if more people are in this situation, I think it's worth to consider the proposed change This example with doc = await Promise.all(items.map(item => item.toDoc(currentUser, options))); works without the need of an |
There's no way to reliably fix the 3rd one, this has been discussed a lot in the past. We can't check the return value of a function and then retroactively not pass a callback to the function. The |
Even with this issue closed, as of the linked issue above, I'd like to re-emphasize on @caub statements here. |
@caub This is the reason function isAsync(fn) {
return fn[Symbol.toStringTag] === 'AsyncFunction';
} This way, it's easier to check if the function returns a promise without actually calling it. But still agree with you that this is against the async/await conventions. As adding an from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function
... and so it'll be useless and worst it will add some performance overhead if the code is transpiled by babel for example as it'll add more code and steps just because of the existence of the
but with this library, async.mapLimit([1], 1, async v => v) and async.mapLimit([1], 1, v => Promise.resolve(v))) are not equivalent! |
The current behavior is very surprising:
Because for short functions, it's tempting to just write
foo: () => doSomething()
. It's very easy to forget theasync
keyword thereBut currently this won't work, it needs to be written as
foo: async () => doSomething()
Could we rather check if a callback is passed or not, instead of checking if the function is an AsyncFunction?
The text was updated successfully, but these errors were encountered: