Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

Use pify or so #2222

Closed
levino opened this issue Jan 27, 2019 · 5 comments
Closed

Use pify or so #2222

levino opened this issue Jan 27, 2019 · 5 comments

Comments

@levino
Copy link
Contributor

levino commented Jan 27, 2019

For example here:

https://github.com/ethereum/web3.js/blob/2cfa2ea184466d52bca62f52c82aa570fd0ad9a9/packages/web3-eth-accounts/src/Accounts.js#L219-L220

There is a lot of boilerplate in every method to handle callbacks and promises at the same time. That is not necessary. You can just write the whole library in one style and then finally wrap everything with pify or something you write yourself. The current "solution" is very dangerous because someone might some day forget to also return the promise.

Also when a callback is provided, I find that no promise schould be returned. Atm you call the callback and return the promise which super funky.

@levino
Copy link
Contributor Author

levino commented Jan 27, 2019

@levino
Copy link
Contributor Author

levino commented Jan 27, 2019

It would go like this:

import callbackify from 'callbackify'

const _someMethod(data) {//returns a Promise
   ...
}

const someMethod(data, callback) {
   if (callback) {
     return callbackify(_someMethod)(data, callback)
   }
   return _someMethod(data)
}

@nivida
Copy link
Contributor

nivida commented Jan 27, 2019

I think this issue can be closed and referenced in the #2224 issue.

@nivida nivida closed this as completed Jan 27, 2019
@levino
Copy link
Contributor Author

levino commented Jan 27, 2019

I disagree. I think #2224 will not be implemented soon and introduce breaking changes. This issue right here is simply about improvement of code quality.

@levino
Copy link
Contributor Author

levino commented Jan 27, 2019

I changed a bit the code above and used callbackify because this will allow to write all code in Promise style and then only convert it to support callbacks too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants