Skip to content
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

feat: use util.promisify when possible #68

Closed
wants to merge 1 commit into from

Conversation

pi0
Copy link

@pi0 pi0 commented Oct 15, 2018

This PR adds an enhancement to prefer using native util.promisify by checking some conditions:

  • Util.promisify supported
  • Native not disabled with options (options.native)
  • No multiArgs mode
  • Is errorFirst mode

These conditions ensure that there is no breaking change while improving perf according to nodejs/node#12442:

Add methods for creating, resolving and rejecting promises using the V8 C++ API that does not require the creation of extra resolve and reject functions to process.binding('util').

Alternative: blindmedia/pify-native -- will be depricated when (!hopefuly) this PR gets merged.

Benchmark results

Closes #41. Closes #62.

@sindresorhus
Copy link
Owner

Sorry for forgetting about this one... I have been thinking about it from time to time, and in the end I have decided this does not make sense. Pify already works fine without util.promisify, it's actually even faster. You would also not be able to use util.promisify internally in all situations, which could lead to unexpected behavior. Depending on util.promisify also makes pify less portable, like for use in the browser.

I think the best solution is to just use util.promisify for the simple cases, and pify if you need more advanced handling or browser support.

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

Successfully merging this pull request may close these issues.

How does this differ from util.promisfy Take advantage of util.promisify() if it lands in Node.js
2 participants