GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — util: add util.promisify() by addaleax · Pull Request #12442 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Apr 16, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

See nodejs/CTC#12 for discussion/background. I would say that that thread is a better place for general discussion, and I prefer it if comments here would be kept to what is directly relevant for this PR.

CI: https://ci.nodejs.org/job/node-test-commit/9143/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

util, timers

Original commit descriptions

util: add internal bindings for promise handling

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

util: add util.promisify()

Add util.promisify(function) for creating promisified functions.

Fixes: nodejs/CTC#12

timers: add promisify support

Add support for util.promisify(setTimeout) and util.promisify(setImmediate) as a proof-of-concept implementation. clearTimeout() and clearImmediate() resolve the promise immediately instead of rejecting it; that might be the most opinionated choice about this.

/cc @chrisdickinson @benjamingr @Fishrock123 @nodejs/ctc

edit: CTC voting comment

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
orizens, yavorski, chrisdickinson, stevenvachon, not-an-aardvark, styfle, blia, dypsilon, zdychacek, zkat, and 31 more reacted with thumbs up emoji MylesBorins, benjamingr, gergelyke, thefourtheye, sindresorhus, marvinhagemeister, styfle, elektronik2k5, aqrln, ChALkeR, and 30 more reacted with hooray emoji refack, felixrieseberg, gillesdemey, bcmarinacci, zkat, benpigchu, fdaciuk, mistadikay, vsemozhetbyt, harpreetkhalsagtbit, and 9 more reacted with heart emoji
All reactions
  • 👍 41 reactions
  • 🎉 40 reactions
  • ❤️ 19 reactions
@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. promises Issues and PRs related to ECMAScript promises. semver-minor PRs that contain new features and should be released in the next minor version. timers Issues and PRs related to timers, setImmediate(), setInterval(), and setTimeout(). util Issues and PRs related to the built-in util module. labels Apr 16, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. timers Issues and PRs related to timers, setImmediate(), setInterval(), and setTimeout(). util Issues and PRs related to the built-in util module. labels Apr 16, 2017
@addaleax

addaleax commented Apr 16, 2017

Copy link
Copy Markdown
Member Author

Also, @benjamingr has a good point about the naming, quote:

optimally I'd hope for most people to not even be aware of what a promise is in 5 years and just use async/await seamlessly :)

We can bikeshed this further here, but just to get an idea how people feel, could people 👍 this comment for “promisify sounds good” and 👎 this for “I’d prefer awaitable or something else” (both assuming that we agree on the approach in this PR in general)?
not-an-aardvark, marvinhagemeister, styfle, elektronik2k5, mattbrunetti, ericmdantas, STRML, golsan, LinusU, bjrmatos, and 28 more reacted with thumbs up emoji ahmedissa reacted with thumbs down emoji
All reactions
  • 👍 38 reactions
  • 👎 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax mentioned this pull request Apr 16, 2017
Support awaiting values through util.awaitable nodejs/CTC#12
Closed
@Fishrock123

Fishrock123 commented Apr 16, 2017

Copy link
Copy Markdown
Contributor

Maybe we should split the timers bits into a second PR? They don't map well to promises since we don't have cancelables. Maybe we could plug the timers props onto the promise itself? Not sure.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Apr 16, 2017

Copy link
Copy Markdown
Contributor

I.e. clearTimeout() should probably work on whatever setTimeoutPromise() returns (a promise but you get the idea), same for Immediates.
refack reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
williamkapke
williamkapke suggested changes Apr 16, 2017
View reviewed changes

@williamkapke williamkapke left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I (also) would like to see the timers impl in a followup PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/util.js Outdated

@williamkapke williamkapke Apr 16, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This truthy test of orig is not needed since it is guaranteed to be a function at this point

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr and addaleax reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@addaleax addaleax Apr 16, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@williamkapke thanks for catching, done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@benjamingr benjamingr self-assigned this Apr 16, 2017
@benjamingr

benjamingr commented Apr 16, 2017

Copy link
Copy Markdown
Member

A big +1 on this. I think this could be a great first step for helping users deal with async/await in their code. There is a clear benefit over a userland implementation, and the need is prevalent.
refack and joshualyon reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax
addaleax force-pushed the promisify branch from c1f7c31 to 162f3c0 Compare April 16, 2017 17:11
@Fishrock123

Fishrock123 commented Apr 16, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Going to expand on my point a bit: I think this is reasonable to do for simple callback APIs but I think we should spend more* time to think about how to wrap more complex APIs such as timers, child processes, etc well.

  • * Might be good for an EP?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax
addaleax force-pushed the promisify branch from 162f3c0 to 506b333 Compare April 16, 2017 17:11
@addaleax

addaleax commented Apr 16, 2017

Copy link
Copy Markdown
Member Author

Maybe we should split the timers bits into a second PR?

If that’s what people here prefer, sure. I just thought it made sense because they’re about the only API that’s a bit icky because they don’t conform to Node’s standard callback pattern, so whenever we introduce something like util.promisify() we’d probably also want to have support for promisified timers (in some way).

I.e. clearTimeout() should probably work on whatever setTimeoutPromise() returns (a promise but you get the idea), same for Immediates.

Makes sense, I’ve updated the PR with isPromise(timer) checks anyway.
styfle reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
williamkapke
williamkapke approved these changes Apr 16, 2017
View reviewed changes
@benjamingr

benjamingr commented Apr 16, 2017

Copy link
Copy Markdown
Member

@Fishrock123

Going to expand on my point a bit: I think this is reasonable to do for simple callback APIs but I think we should spend more time to think about how to wrap more complex APIs such as timers, child processes, etc well.

I definitely agree - we need a proper promised core at some point - but #5020 and similar attempts have stagnated and quoting myself in the original issue:

The idea of util.awaitable vs a promise core is that it's hopefully a lot less objectionable, it poses a lot less controversy, it's a lot less opinionated and it means exposing a capability that users can build on and not an API.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Apr 16, 2017

Copy link
Copy Markdown
Member Author

I think @Fishrock123’s comment was about the timers stuff, not the general API here? I feel inclined to disagree that this needs an EP, though…
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Apr 16, 2017

Copy link
Copy Markdown
Contributor

Correct. I only mean for how to wrap any APIs that are more complex than single events or callbacks and which do not wrap as well.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TimothyGu
TimothyGu reviewed Apr 16, 2017
View reviewed changes
Comment thread doc/api/util.md Outdated

@TimothyGu TimothyGu Apr 16, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function isn't a legal identifier in JS, and for consistency we probably want to keep it legal.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread doc/api/util.md Outdated

@TimothyGu TimothyGu Apr 16, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

* {symbol}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread doc/api/util.md Outdated

@TimothyGu TimothyGu Apr 16, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lower case "custom"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Apr 16, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the uppercase variant is okay here, it’s referring to a literal section title, and it matches what we do for the other two section references in this file. If you feel strongly about it I’ll change it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TimothyGu reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread doc/api/timers.md Outdated

@TimothyGu TimothyGu Apr 16, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An async function example would be cool as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax, vkurchatkin, and brandonsueur reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
Comment thread doc/api/timers.md Outdated

@TimothyGu TimothyGu Apr 16, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to clarify setImmediate() or its promisified equivalent.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread doc/api/timers.md Outdated

@TimothyGu TimothyGu Apr 16, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread doc/api/util.md Outdated

@TimothyGu TimothyGu Apr 16, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Using the util.promisify.custom symbol"?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread lib/internal/util.js Outdated

@TimothyGu TimothyGu Apr 16, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the prototype of orig?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Apr 16, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu yeah … that thought occurred to me, but I basically didn’t see how setting it would make sense here. If you prefer, I’ll add

Object.setPrototypeOf(fn, Object.getPrototypeOf(orig));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
williamkapke reacted with confused emoji
All reactions
  • 😕 1 reaction

@TimothyGu TimothyGu Apr 16, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't see how it would matter that much in real world, but having that there does seem a bit more natural to me. Up to you if you want to set it or not.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TimothyGu TimothyGu Apr 16, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would matter that much; it just occurred to me having the prototype being equivalent might be more natural. Up to you.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Apr 16, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu I’ve included it for now, let’s see what people think.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@addaleax
addaleax force-pushed the promisify branch 3 times, most recently from ad34f91 to fe27d6a Compare April 16, 2017 17:40
@addaleax

addaleax commented Apr 16, 2017

Copy link
Copy Markdown
Member Author

@TimothyGu Addressed most of your comments, PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vkurchatkin
vkurchatkin reviewed Apr 16, 2017
View reviewed changes
Comment thread lib/timers.js Outdated

@vkurchatkin vkurchatkin Apr 16, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a bad idea for many reasons. I think it's fine if you can't cancel promisified timeout until we have some generic cancellation mechanism.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Apr 16, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a bad idea for many reasons.

Would you care to provide some? Otherwise this is just not helpful.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vkurchatkin vkurchatkin Apr 16, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, attaching properties to native objects seems like a bad idea from performance perspective. Secondly, this implements ad hoc cancellation mechanism, while there is no spec on standards track or even community consensus. This approach suffers from the fact that it's not propagated and there it doesn't play nice with async-await.

I think it's fine to make promisified versions not cancellable. If you use them you are most likely not going to cancel anyway.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Apr 16, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, attaching properties to native objects seems like a bad idea from performance perspective.

That’s true, but it’s opt-in, right?

This approach suffers from the fact that it's not propagated

What do you mean by propagated?

But yeah, I would be okay with dropping clear* support for these, at least for the time being.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vkurchatkin vkurchatkin Apr 16, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by propagated?

Something like this:

const promise = setTimeout(1000)
  .then(() => ...);
clearTimeout(promise); // This won't work, you need original promise

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax, ChALkeR, and LinusU reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

@jasnell jasnell Apr 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to agree with @vkurchatkin on this. Until there is a standard way of canceling promises, we likely shouldn't do this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
yunong reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@addaleax addaleax Apr 19, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vkurchatkin @jasnell Okay, I’ve dropped clearTimeout/clearImmediate support. It’s a niche feature anyway and it should be easy to readjust later if we want.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@addaleax
addaleax force-pushed the promisify branch from fe27d6a to 9d08d10 Compare April 16, 2017 17:58
addaleax and others added 7 commits May 9, 2017 15:01
@addaleax
util: add util.promisify()
99da8e8 
Add `util.promisify(function)` for creating promisified functions.
Includes documentation and tests.

Fixes: nodejs/CTC#12
PR-URL: nodejs#12442
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: William Kapke <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
@MadaraUchiha @addaleax
test: add a bunch of tests from bluebird
3ea2301 
Take tests from Bluebird's promisify's tests and adapted them to the
format in use here.
Add tests making sure things work with async functions.
Add basic usability tests.

PR-URL: nodejs#12442
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: William Kapke <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
@giltayar @addaleax
test: add test for promisify customPromisifyArgs
e965ed1 
PR-URL: nodejs#12442
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: William Kapke <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
@addaleax
timers: add promisify support
e7c5145 
Add support for `util.promisify(setTimeout)` and
`util.promisify(setImmediate)` as a proof-of-concept implementation.
`clearTimeout()` and `clearImmediate()` do not work on those Promises.
Includes documentation and tests.

PR-URL: nodejs#12442
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: William Kapke <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
@addaleax
fs: support util.promisify for fs.read/fs.write
fbcb4f5 
PR-URL: nodejs#12442
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: William Kapke <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
@addaleax
child_process: support promisified exec(File)
fe5ca3f 
Author: Benjamin Gruenbaum <[email protected]>
Author: Anna Henningsen <[email protected]>

PR-URL: nodejs#12442
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: William Kapke <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
@addaleax
dns: support promisified lookup(Service)
faf6654 
PR-URL: nodejs#12442
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: William Kapke <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
@addaleax
addaleax force-pushed the promisify branch from a21afd7 to faf6654 Compare May 9, 2017 13:02
@addaleax

addaleax commented May 9, 2017

Copy link
Copy Markdown
Member Author

Landed in f72376d...faf6654

Thanks everyone for the reviews and support!
styfle, thefourtheye, aqrln, iamstarkov, ljharb, vsemozhetbyt, ChALkeR, TimothyGu, iraklisg, and noopnik reacted with thumbs up emoji styfle, refack, skcc75, novemberborn, thefourtheye, aqrln, calvinmetcalf, joyeecheung, iamstarkov, ljharb, and 2 more reacted with hooray emoji styfle, benjamingr, thefourtheye, aqrln, bnb, iamstarkov, capaj, ljharb, vsemozhetbyt, and ChALkeR reacted with heart emoji
All reactions
  • 👍 10 reactions
  • 🎉 12 reactions
  • ❤️ 10 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

benjamingr commented May 9, 2017

Copy link
Copy Markdown
Member

@addaleax going to give a small talk about this journey on Thursday, starting in nodejs/node-v0.x-archive#7642

Thanks a ton for taking up the glove and working together on the conceptual side, while undertaking the vast majority of the technical burden. Props for the execution and delivery, and thank you :)
capaj, vsemozhetbyt, barakd, TimothyGu, styfle, and MadaraUchiha reacted with hooray emoji addaleax, thefourtheye, styfle, paulirish, iamstarkov, vsemozhetbyt, Zirak, and MadaraUchiha reacted with heart emoji
All reactions
  • 🎉 6 reactions
  • ❤️ 8 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye

thefourtheye commented May 9, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@MadaraUchiha You were right! This PR is @benjamingr's birthday present 😉
MadaraUchiha, benjamingr, addaleax, iamstarkov, vsemozhetbyt, Zirak, and TimothyGu reacted with hooray emoji
All reactions
  • 🎉 7 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell mentioned this pull request May 11, 2017
8.0.0 Release Proposal #12220
Closed
@ljharb

ljharb commented May 30, 2017

Copy link
Copy Markdown
Member

I just published https://www.npmjs.com/package/util.promisify (with code largely copy/pasted from this PR, with permission from @addaleax) for those interested.
BridgeAR, SimenB, styfle, and ognus reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@divyenduz divyenduz mentioned this pull request Jun 16, 2017
Support Promise redis/node-redis#864
Closed
@TimothyGu TimothyGu mentioned this pull request Jun 30, 2017
Expose reference to the Symbol(customPromisifyArgs) #14007
Closed
syrnick
syrnick reviewed Jul 18, 2017
View reviewed changes
Comment thread lib/internal/util.js
promiseResolve(promise, values[0]);
}
});
} catch (err) {

@syrnick syrnick Jul 18, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch may accidentally catch errors thrown from promiseReject if orig.call happened to be synchronous. E.g. the value was cached in memory and didn't need a remote lookup. promiseReject on line 264 will be a no-op in that case.

Ideally if the execution hit line 253 and then 264, it should re-throw the error instead of no-oping. At least it could log a warning.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ljharb ljharb Jul 18, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing way is definitely better; a function that returns a promise should never throw, only return a rejected promise.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr, addaleax, and ChALkeR reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

@benjamingr benjamingr Jul 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syrnick can you please open an issue about it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ljharb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@ljharb ljharb Jul 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also, since a promise can only represent one value; new Promise((resolve, reject) => { reject(1); reject(2); }) is supposed to have the second rejection be a noop, and does not throw the 2, so it'd be important for util.promisify to maintain parity)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@benjamingr benjamingr mentioned this pull request Aug 16, 2017
Promise Performance Meeting nodejs/promises#31
Open
@benjamingr benjamingr mentioned this pull request Sep 1, 2017
Allow passing objects to util.promisify nodejs/CTC#174
Closed
@TimothyGu TimothyGu mentioned this pull request Sep 17, 2017
meta: planning for promisified fs and crypto #15413
Closed
@merisbahti merisbahti mentioned this pull request Oct 19, 2017
Just a comment merisbahti/promiser.js#1
Open
syrnick
syrnick reviewed Jan 31, 2018
View reviewed changes
Comment thread lib/internal/util.js
};

const kCustomPromisifiedSymbol = Symbol('util.promisify.custom');
const kCustomPromisifyArgsSymbol = Symbol('customPromisifyArgs');

@syrnick syrnick Jan 31, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for keeping this private? It's obviously useful in a number of places.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ljharb ljharb Jan 31, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/nodejs/node/pull/12442/files#diff-a43208147d795be6dd3517c53226e37dR280 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@syrnick syrnick Jan 31, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that's still on internal/util, not accessible to user code. The other one(kCustomPromisifiedSymbol) is available as util.promisify.custom.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TimothyGu TimothyGu Jan 31, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syrnick See #14007.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pi0 pi0 mentioned this pull request Oct 15, 2018
feat: use util.promisify when possible sindresorhus/pify#68
Closed
@P-Seebauer P-Seebauer mentioned this pull request Mar 24, 2019
util.callbackify return value problematic characteristics #26890
Closed
@samikay101 samikay101 mentioned this pull request Mar 10, 2025
feat(vscode): Add function to create and update solution file with Logic App .csproj Azure/LogicAppsUX#6721
Merged
2 of 3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR left review comments
@ljharb ljharb ljharb approved these changes
@benjamingr benjamingr benjamingr approved these changes
@jasnell jasnell jasnell approved these changes
@mcollina mcollina mcollina approved these changes
@Fishrock123 Fishrock123 Awaiting requested review from Fishrock123
+11 more reviewers
@littledan littledan littledan left review comments
@syrnick syrnick syrnick left review comments
@vkurchatkin vkurchatkin vkurchatkin left review comments
@thefourtheye thefourtheye thefourtheye left review comments
@MadaraUchiha MadaraUchiha MadaraUchiha left review comments
@SimenB SimenB SimenB left review comments
@TimothyGu TimothyGu TimothyGu approved these changes
@williamkapke williamkapke williamkapke approved these changes
@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes
@evanlucas evanlucas evanlucas approved these changes
@MylesBorins MylesBorins MylesBorins approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@Fishrock123 Fishrock123

@benjamingr benjamingr

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. notable-change PRs with changes that should be highlighted in changelogs. promises Issues and PRs related to ECMAScript promises. semver-minor PRs that contain new features and should be released in the next minor version. timers Issues and PRs related to timers, setImmediate(), setInterval(), and setTimeout(). util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support awaiting values through util.awaitable

Uh oh!

There was an error while loading. Please reload this page.

20 participants

@addaleax @Fishrock123 @benjamingr @vkurchatkin @thefourtheye @misterdjules @jasnell @giltayar @yunong @littledan @Daniel15 @ChALkeR @ljharb @MylesBorins @indutny @fhinkel @markstos @mcollina @aendra-rininsland @styfle
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.