Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Adopt async/await and promises instead of callbacks #1311

Closed
fsdiogo opened this issue Apr 13, 2018 · 9 comments
Closed

Adopt async/await and promises instead of callbacks #1311

fsdiogo opened this issue Apr 13, 2018 · 9 comments
Labels
exploration P3 Low: Not priority right now

Comments

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 13, 2018

I'm opening this issue so we can have a discussion about adopting async/await and promises instead of callbacks across the js repos.

You can check ipfs/notes#289 to see where this came up.

  • We could start to adopt async/await and promises, as the JavaScript ecosystem is evolving in that direction
    • This is a never ending discussion, but in my opinion it makes the code more readable and new developers will probably have a better understanding of the source code

As Node 6 is almost entering maintenance phase, I think we're right on time to at least start chatting about this.

I'm all for async/await, to be completely honest I really don't like the callback style 😅

My idea is not to make a huge refactor to abolish callbacks right away, at least not right now, but in future PRs with new features we could start to shift to the promises style.

What do you guys think?

@vasco-santos
Copy link
Member

I agree that adopting async/await would improve considerably the readability of the code.

Moreover, we would be able to handle both sync and async errors within the same block, which would also turn the error handling easier.

@dryajov
Copy link
Member

dryajov commented Apr 13, 2018

I generally like the idea of using async/await (and generators, generics, types, etc.. modern JS FTW ;)). My only concern at this point is that we'd be mixing two completely different styles, which could get ugly pretty quickly. I think we have to be careful and come up with a plan similar to the one being undertaken with #1260.

I would evaluate every new feature that is immediately available to us on each platform and where/how we can use it to improve our current implementation.

We also have to take into consideration libraries such as pull-stream that we rely on extensively and how to mix it with the more async/await'y stuff. There is an issue in the pull-stream repo that discussed how to bring it to modern js.

Finally, I'll play the devil's advocate and mention that http://caolan.github.io/async/ has a very complete set of async primitives that alleviate most of the hurdles associated with callbacks.

@pgte
Copy link
Contributor

pgte commented Apr 13, 2018

As much as I was against promises and pro callbacks, I'm fully converted now to async/await. In comparison, callbacks are clunky and harder to read.

Pull-streams use callbacks in some places (like in pull.asyncMap) but there are alternatives like pull-promise-map we could evaluate to use while pull-stream decides.

@pgte pgte closed this as completed Apr 13, 2018
@pgte pgte reopened this Apr 13, 2018
@daviddias
Copy link
Member

Please make a complete proposal with things like:

  • Pros/Cons
  • Studies that show on how async/await improved the dev process and the readability of the code
  • Performance Impact

Essentially, justify the time investment with something stronger than an opinion. Have in mind that there are far more important tasks at hand.

My idea is not to make a huge refactor to abolish callbacks right away, at least not right now, but in future PRs with new features we could start to shift to the promises style.

Either commit to A or B, inconsistency leads to confusion.

@daviddias daviddias added the P3 Low: Not priority right now label Apr 14, 2018
@justinmchase
Copy link

Personally, I think the callback style is better. Using the async library there is nothing you can't do and if you use await there are still certain kinds of control flow which can be really tricky, such as more complex async tasks like parallelism and queue/cargo situations. You still end up needing to use callbacks or wrapping a bunch of stuff in promisify functions to get await to work and then you just end up with confusing mixed code and indirection. There's also a perf cost.

The callback style code isn't really that hard to read or understand, it seems like developers who are used to doing things linearly or imperatively struggle for a while but the real problem is just learning to think async not the code or the syntax.

Overall the async / await syntax was probably an unneeded addition to javascript and makes the code more confusing ultimately. I already saw someone in this thread say it will be easier to mix sync and async functions, which in my opinion is a problem. It would be better to expose those sync functions as bugs that should be resolved rather than make it easier to not notice them.

@alanshaw alanshaw added the status/deferred Conscious decision to pause or backlog label May 30, 2018
@mikeal
Copy link
Contributor

mikeal commented Jun 25, 2018

So, what I'd like a little guidance on is "what is the policy for new things." I don't think it's realistic, or worth it, to dedicate cycles to re-writing lots of working code that uses callbacks. But being that all the reference implementations are written using callbacks it implies that people should not write new modules using promises and async/await.

Many of the top level API's for modules support both callbacks and promises, but some do not. Is there a policy regarding this? Would it be fine to write a new module that only exposed a promise interface?

@daviddias
Copy link
Member

There are some discussions and notes from discussions we had 1+ years ago but the summary of those are:

  • We expose Promises, Callbacks, Readable Streams and Pull Streams at the top level, i.e. js-ipfs
  • All internals use callbacks and/or pull-streams
  • js-libp2p & js-ipld although being the top level of libp2p and IPLD respectively, they still have been considered internals and so we don't expose multiple APIs.

More context:

@mikeal
Copy link
Contributor

mikeal commented Jun 25, 2018

@diasdavid thanks for the pointers, i'll take a look and maybe we can revisit this all in Berlin :)

@alanshaw
Copy link
Member

alanshaw commented Nov 1, 2018

This endeavour can now be tracked here #1670

@alanshaw alanshaw closed this as completed Nov 1, 2018
@ghost ghost removed the status/deferred Conscious decision to pause or backlog label Nov 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exploration P3 Low: Not priority right now
Projects
None yet
Development

No branches or pull requests

8 participants