goldbergyoni / nodebestpractices

:white_check_mark: The Node.js best practices list (July 2024)
https://twitter.com/nodepractices/
Creative Commons Attribution Share Alike 4.0 International
99.45k stars 10.12k forks source link

Misleading advice: 3.11 Use Async Await, avoid callbacks #428

Closed dmitriz closed 4 years ago

dmitriz commented 5 years ago

This seems like a way too generalist advice:

This is a new way of dealing with asynchronous code which supersedes callbacks and promises.

I think it is misleading to say it "supersedes". Promises and async-await (basically a sugar for promises) carry several overheads that callbacks don't have.

The best gift you can give to your code is using async-await which provides a much more compact and familiar code syntax like try-catch

This feels excessively emotional and subjective. Wrapping code into try/catch patterns and adding async/await tokens can make code longer and more verbose, rather than more compact.

Otherwise: Handling async errors in callback style is probably the fastest way to hell - this style forces to check errors all over, deal with awkward code nesting and makes it difficult to reason about the code flow

This only applies to the Node API callback style with error as first argument. It does not apply to callbacks in general, it does not apply e.g. to error handling in separate callbacks.

goldbergyoni commented 5 years ago

@dmitriz Welcome aboard.

Let's first clarify, callbacks can indeed mean 2 different things to different people:

  1. A code-style for any async function (e.g. fs.readFileContent(filename, callback)) which the user code all over his app
  2. A generic technique to notify the caller on certain events, like in event-emitters

Which is not an anti-pattern: 1, 2, both?

async-await can't be used at all

That's is theoretically doable now with async-generators

dmitriz commented 5 years ago

Welcome aboard.

Thank you!

A code-style for any async function (e.g. fs.readFileContent(filename, callback)) which the user code all over his app

I would say it depends on your circumstances, all over or occasionally - whatever suits the user :)

Which is not an anti-pattern: 1, 2, both?

None of them is anti-pattern at this level of generality. Each has its uses and strengths.

async-await can't be used at all

That's is theoretically doable now with async-generators

That would be async-await + extra stuff :) While plain callback functions do job without any other stuff. But why making things easy if we can make them hard :)

dmitriz commented 5 years ago

I should have said, I love your work here and many advises are really helpful. But I am worried about the generalists advices that are too easy to misinterpret.

The callbacks in Node.js provide the simplest implementation of the Continuation-Passing-Style, a fundamental programming pattern with long history and extensive research behind. When used correctly, callbacks can provide simple uniform interface for dealing with arbitrary async operations, whether it is delivering a single value like promise or stream of values.

Promises or async/await provide a boilerplatte around only the subset of callbacks of limit scope that can only return one value only once. It has its uses but in real life things are constantly changing, so not being able to cancel or update the value or send a new one is a serious limitation. Using generators and async generators is possible but adds considerable complexity.

Promises do not rely on and are in fact in violation of patterns with long history and solid research.

goldbergyoni commented 5 years ago

@dmitriz Sorry for the late reply, I'm traveling these days hence my limited availability. I respect your feedback and always trying to learn, at the same time I highly disagree.

As you alluded, there are 2 types of callback usage:

  1. Single value return - ~95% of a typical project code (a function returns a single value). this can obviously get replaced with async-await. If you use callbacks for your single-value return functions you'll end up with a code that is dramatically less maintainable than async-await. I've seen this so many times, horrors, and any of my customers who ported it to async/await bought me then flowers. Should I really paste here an example how 10 long async-await function turns into unreadable ball of mud where each returned value includes its own personal potential error? just Google 'callback hell'... Should I choose one advice for this guide, just one, it would be - don't use callbacks as your main async mechanism for single-return functions

  2. Stream of values - this can't be replaced easily with async/await and I'm 100% comfortable with using callbacks here as part of evnet emitter. Why? lack of other options, small code, usually just a single callback

someEmitter.on('event' , () => {
//do something
}

It has its uses but in real life things are constantly changing, so not being able to cancel or update the value or send a new one is a serious limitation

Why callbacks are more convenient for tapping into the async operation than promises? In any case, I would claim those are rare cases, can you share examples that show that those needs are common? if you refer to 5 loc per project, then yeah, callbacks are fine. I'm not worried about 3 callbacks, I'm super worried about 1000 callbacks.

But I am worried about the generalists advices that are too easy to misinterpret

Can you please share other examples?

stale[bot] commented 4 years ago

Hello there! 👋 This issue has gone silent. Eerily silent. ⏳ We currently close issues after 100 days of inactivity. It has been 90 days since the last update here. If needed, you can keep it open by replying here. Thanks for being a part of the Node.js Best Practices community! 💚

dmitriz commented 4 years ago

Hi @goldbergyoni Apologies for somehow missing your response (that I greatly appreciate!) and only noticed it now that it was closed by the bot.

Single value return - ~95% of a typical project code (a function returns a single value). this can obviously get replaced with async-await. If you use callbacks for your single-value return functions you'll end up with a code that is dramatically less maintainable than async-await.

Only if you use callbacks badly. Promise is basically a wrapper with diminished functionality and overheads around functions accepting 2 callbacks. Even if you don't see these functions, the package or API wraps them into promises for you. But underneath there are still callback-based functions. So you ARE using callbacks even when it looks like Promises. The difference is merely syntactic and nothing prevents you from writing a better Promise wrapper with exactly the same syntax but without running and unwrapping functionality that can have a performance cost in the real-life applications.

I have this library providing such wrapper that is doing much more: https://github.com/dmitriz/cpsfy

You can either import my library or copy-paste my code (very short). Then use my CPS wrapper instead of Promise. So again, underneath is a callback-based function, but you use it with a wrapper to make your syntax as manageable as with promises.

Async/await is basically a different syntax to still write Promises, with the same diminished functionality and overheads. I personally don't like the noise from introducing the foreign keywords like async/await into my functions, as these make them less compatible with general-purpose function handling tests or other code.

I've seen this so many times, horrors, and any of my customers who ported it to async/await bought me then flowers. Should I really paste here an example how 10 long async-await function turns into unreadable ball of mud where each returned value includes its own personal potential error? just Google 'callback hell'... Should I choose one advice for this guide, just one, it would be - don't use callbacks as your main async mechanism for single-return functions

If you look at my documentation pages, there is a rewritten example like that with my wrapper. The best advice people appreciate is information not pressing opinion onto them. Tell them about different options, advantages and disadvantages if you like but again, rather objective information than opinions.

There are too many opinions out there without arguments and discussions rather than objective helpful information.

Stream of values - this can't be replaced easily with async/await and I'm 100% comfortable with using callbacks here as part of evnet emitter. Why? lack of other options, small code, usually just a single callback

So you are using callbacks directly, without wrappers, but then you run into exactly the same problems you mentioned yourself right? Nesting is a mess. However my wrappers work uniformly with event listeners, so you can have the same cleaner syntax as with promises.

Why callbacks are more convenient for tapping into the async operation than promises?

Callbacks inside wrappers are more convenient.

In any case, I would claim those are rare cases, can you share examples that show that those needs are common? if you refer to 5 loc per project, then yeah, callbacks are fine. I'm not worried about 3 callbacks, I'm super worried about 1000 callbacks.

Please look at my library for many examples. Will appreciate any feedback!