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

Add context option to utils.promisify() for method binding #13338

Closed
wesbos opened this issue May 31, 2017 · 13 comments
Closed

Add context option to utils.promisify() for method binding #13338

wesbos opened this issue May 31, 2017 · 13 comments
Labels
feature request Issues that request new features to be added to Node.js. promises Issues and PRs related to ECMAScript promises. util Issues and PRs related to the built-in util module.

Comments

@wesbos
Copy link

wesbos commented May 31, 2017

  • Version 8:
  • Platform OSX:

The new utils.promisify() will promisify methods, but they lose their this context. Many libs require their methods to be bound.

Here is an example I'm working on with the offical Stripe package.

  stripe.refunds.create(refundOptions, (err, refund) => {
    if (err) {
      // handle error..
      return;
    }
    console.log(refund);
  });

If I want to promisify this:

// currently have to do
const createRefund = util.promisify(stripe.refunds.create);
const refund = await createRefund.call(stripe.refunds, refundOptions);

// or
const createRefund = util.promisify(stripe.refunds.create).bind(stripe.refunds);
const refund = await createRefund(refundOptions);

Ideally I would do this:

const createRefund = util.promisify(stripe.refunds.create, stripe.refunds);
const refund = await createRefund(refundOptions);

I've been using the ES6-promisify package for a while now, and it has this option. The Bluebird promisify method also has an option for this.

@Fishrock123 Fishrock123 added feature request Issues that request new features to be added to Node.js. promises Issues and PRs related to ECMAScript promises. util Issues and PRs related to the built-in util module. labels May 31, 2017
@oaleynik
Copy link

@wesbos I believe all Stripe methods return promises if callback is not provided https://github.com/stripe/stripe-node#using-promises

But idea of context for promisify makes sense.

@wesbos
Copy link
Author

wesbos commented May 31, 2017

Oh - I didn't know that! Awesome.

But yes - I've needed this for many other libs that don't offer promises.

@seishun
Copy link
Contributor

seishun commented Jun 3, 2017

@wesbos how do you imagine this should work in the following case?

var o1 = {a: true};
var o2 = {a: false};
function foo(cb) { cb(null, this.a); }
o2.f = util.promisify(foo, o1);
o2.f().then(console.log);

@seishun
Copy link
Contributor

seishun commented Jun 8, 2017

The PR implementing this got negative response so I guess we should close both the PR and this issue. cc @nodejs/collaborators objections?

@benjamingr
Copy link
Member

Hi, I don't think we should mess with contexts, the user can reassign the function to the object and context will work (promisify has tests for this).

o.a = util.promisify(o.b);
o.a(); // `this` is o

@seishun
Copy link
Contributor

seishun commented Jun 9, 2017

Closing due to lack of support, see also #13440.

@seishun seishun closed this as completed Jun 9, 2017
@doasync
Copy link

doasync commented Jul 31, 2017

You can promisify all object methods without object modification using util.promisify and ES6 Proxy.
Use it like this:
doAsync(fs).readFile('package.json', 'utf8').then(result => {...})
No extra variables and duplicated objects (WeakMap is used). Check this package: https://github.com/doasync/doasync

@TimothyGu
Copy link
Member

(Just a note, Proxies are cool but pretty slow.)

@doasync
Copy link

doasync commented Aug 1, 2017

(Just a note, Proxies are cool but pretty slow.)

I think pify implementation will be slower, but we should test.

@roychri
Copy link

roychri commented Dec 19, 2018

Hi, I don't think we should mess with contexts, the user can reassign the function to the object and context will work (promisify has tests for this).

o.a = util.promisify(o.b);
o.a(); // `this` is o

Be careful with this, doing something like:

fs.readFile = util.promisify(fs.readFile);

while other packages use fs.readFile... it will mess things up.

@hotohoto
Copy link

hotohoto commented May 9, 2019

Applying the native way as described in https://github.com/doasync/doasync, I guess we could do this.

const func2 = util.promisify(func)
const ret = await func2.apply(context, [arg1, arg2, ...])

@jdmarshall
Copy link

For future generations reading this:

let openEntry = util.promisify(zip.stream).bind(zip);

is side-effect free and still one line.

@benjamingr
Copy link
Member

@roychri for posterity - we offer fs.promises for that :]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. promises Issues and PRs related to ECMAScript promises. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants