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

Allow promisification of a single object method: #26

Conversation

jamestalmage
Copy link

This allows opts to be a string. If so, it will promisify the named function of input and return a thunk with this bound to input.

This is really handy for chainable structures, where binding this can get a little verbose:

var bundler = browserify()
  .require(file)
  .transform(brfs)
  .exclued(excluded);

return pify(bundler.bundle.bind(bundler))();

becomes

return pify(
  browserify()
    .require(file)
    .transform(brfs)
    .exclued(excluded),
  'bundle'
)();

@SamVerschueren
Copy link
Contributor

It was discusses a couple of times in other issues #23. But we can always reopen the discussion :).

I might find the first example clearer though.

@schnittstabil
Copy link
Contributor

schnittstabil commented Jun 12, 2016

There's a nice ES proposal for a ::-operator in this context (already available as bable plugin):

return pify(
  browserify()
    .require(file)
    .transform(brfs)
    .exclued(excluded)::bundle
)();

respectively:

return pify(
  browserify()
    .require(file)
    .transform(brfs)
    .exclued(excluded)
)::bundle();

@@ -37,6 +37,10 @@ var pify = module.exports = function (obj, P, opts) {
P = Promise;
}

if (typeof opts === 'string') {
return processFn(obj[opts], P, {}).bind(obj);
Copy link
Contributor

@schnittstabil schnittstabil Jun 12, 2016

Choose a reason for hiding this comment

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

I think the intention of pify(x, 'foo') isn't obvious, should pify bind the promisified foo function to the non-promisified or the (new) promisified object – both are reasonable, at least sometimes.

return processFn(obj[opts], P, {}).bind(obj);
// vs.
var objP = processFn(obj, P, {});
return objP[opts].bind(objP);

@jamestalmage
Copy link
Author

The :: looks promising, though I am not sure you're using it correctly in either of your examples. I think you would need the ::obj.fn variant in this case.

The example from #23 doesn't illustrate situations where extracting the this value requires an extra variable declaration to store the reference.

@sindresorhus
Copy link
Owner

What if you want to use methodName, but need to specify an option? I think this should be an option instead, if we choose to go with this.

@sindresorhus
Copy link
Owner

How about a bind option that instead binds all properties in the passed object to the object?

return pify(
  browserify()
    .require(file)
    .transform(brfs)
    .excluded(excluded),
{bind: true}).bundle();

It could accept true or an object to bind to.

@schnittstabil
Copy link
Contributor

@jamestalmage If you are curious about, ::obj.func and obj::func are identically interpreted.

@jamestalmage
Copy link
Author

::obj.func and obj::func are identically interpreted.

Not true, they will be interpreted as obj.func.bind(obj) and func.bind(obj) respectively.

function bar() {
 console.log(this.foo + 'bar1');
}

var foo = {
  foo: 'foo',
  bar: function () {
    console.log(this.foo + 'bar2');
  }
};

setImmediate(foo::bar);
//=> foobar1;

setImmediate(::foo.bar);
//=> foobar2;

How about a bind option that instead binds all properties in the passed object to the object

I don't see a whole lot of point to bind:true, when would you ever want it to be false? I would think, if promisifying an object, all methods should be bound to the original object. Not doing so could cause lots of subtle bugs:

var obj = {
  coreMethod(cb) {
    // does something before calling cb
  },
  wrapperMethod(cb) {
    // `this` needs to be the original once promisified, or we will call the promisified method here
    this.coreMethod(function(err, result) {
      if (err) {
        cb(err);
        return;
      }
      // do some more stuff before calling cb
    }
  }
}

@SamVerschueren
Copy link
Contributor

It could be set to true by default?

@schnittstabil
Copy link
Contributor

@jamestalmage Thank you, you're right, good catch 👍

I don't see a whole lot of point to bind:true, when would you ever want it to be false?

IMO, bind:true is a valid default. One may need bind:false to promisify mixins – however, I think this is a pretty hypothetical scenario:

var legacyMixin = {
  bars: function (cb) {
    cb(null, this.foo + ' bars');
  }
};

class C {
  constructor(foo) {
    this.foo = foo;
  }

  foobar() {
    return Promise.resolve('foobar');
  }
}

Object.assign(C.prototype, pify(legacyMixin, {bind: false}));

(new C(42)).bars().then(console.log);
(new C(42)).foobar().then(console.log);

@jamestalmage jamestalmage mentioned this pull request Jun 18, 2016
@sindresorhus
Copy link
Owner

Closing in favor of #27.

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.

4 participants