nodejs / CTC

Node.js Core Technical Committee & Collaborators
80 stars 27 forks source link

Allow passing objects to `util.promisify` #174

Closed benjamingr closed 6 years ago

benjamingr commented 6 years ago

This is a proposal to extend util.promisify to accept objects in addition to promises.

Currently, a user has to type util.promisify for every function in a module they're using. This leads users to prefer userland solutions like bluebird which provide a more ergonomic method that converts entire APIs at once.

This behavior was initially planned for https://github.com/nodejs/node/pull/12442 but we decided to defer it until the initial function behavior was used for a while and we saw the response.

So far the response I've received from communities I polled was generally very positive for promisify, and people often asked about the object variant.

I think now would be a good time to add it:

const fs = util.awaitable(require(fs));
async function foo() {
  const result = await fs.readFile("hello.txt"); // file contains "World"
  console.log("Hello", result);
}
foo(); // logs `Hello World`

Pinging @addaleax who wrote the code for util.promisify and did a considerable amount of design for it.

I'm posting it here for discussion about it and as a place for bikeshedding syntax or making objections for possible designs.

jasnell commented 6 years ago

While I definitely get the utility of this, there's a danger here in that the API could end up doing entirely the wrong thing for functions that do not follow the error back pattern and do not provide their own custom conversion logic.

For instance, what should the result of the following be:

class Foo {
  a(err, arg) {}
  b(num, options) {}
}

const foo = util.awaitable(new Foo());

foo.b(1, {}); // ?? Is this awaitable?

If we can clearly define the semantics and what the users would expect, then I'm all for this.

benjamingr commented 6 years ago

@jasnell it would be identical to what util.promisify does for each property of the object. Something like:

for(const prop of Object.getOwnPropertyNames(Foo.prototype)) {
  if(prop === 'constructor') continue; // .prototype.constructor
  const old = Foo.prototype[prop];
  Foo.prototype[`${prop}Async`] = util.promisify(old); // should probably be defineProperty
}

Or alternatively without modifying the original object.

mcollina commented 6 years ago

I'm generically 👎 on overly overloaded functions. This seems too much, can we do it in a different function?

Moreover, I do not see why it cannot exist in the ecosystem now that the basics of util.promisify  are in core (and implemented in C++).

addaleax commented 6 years ago

I'm generically 👎 on overly overloaded functions. This seems too much, can we do it in a different function?

Seconded

Moreover, I do not see why it cannot exist in the ecosystem now that the basics of util.promisify are in core (and implemented in C++).

I see a single reason, but maybe it’s a biggie (maybe not): It solves the problem @jasnell is pointing out for core modules in a way that can’t be done in userland without continuously keeping up to date with core, because it would allow us to differentiate between the methods on core modules/objects that could reasonably be promisified vs those who can’t. (Simple example: WritableStream#write() can be promisifed, WritableStream#setDefaultEncoding() cannot).

That might still be a lot of work, but it should be pretty doable.

mcollina commented 6 years ago

I see a single reason, but maybe it’s a biggie (maybe not): It solves the problem @jasnell is pointing out for core modules in a way that can’t be done in userland without continuously keeping up to date with core, because it would allow us to differentiate between the methods on core modules/objects that could reasonably be promisified vs those who can’t. (Simple example: WritableStream#write() can be promisifed, WritableStream#setDefaultEncoding() cannot).

I agree that it is a lot of work. However, I think we should provide some strategy to provide awaitable functions for the main modules of core. We might make that mechanism public, but it might be something that should be very specific to core.

I would reverse the process:

  1. design a new internal utility to get a promisified version of a core module
  2. release that promisified layer as experimental
  3. make the utility public once its proven to work for us when we lift the experimental flag
Trott commented 6 years ago

I'm going to close this, but feel free to open another issue in a relevant active repository (TSC perhaps?) and include a link back to this issue if this is a subject that should receive continued attention. Recent governance changes mean this repo probably shouldn't be used anymore. Sorry for the inconvenience.