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 bind option #27

Closed
wants to merge 5 commits into from
Closed

Conversation

jamestalmage
Copy link

@jamestalmage jamestalmage commented Jun 17, 2016

By default, it binds functions to the original object.

If opts.bind is false. It does not bind to the original object, and copies over all non-function properties.

Closes #26.

@jamestalmage
Copy link
Author

See the first commit.

There I added accessor methods that delegate to the bound object. That seems like overkill, still it might be nice:

var x = {
  option: 'foo',
  method: function(cb) {
    setImmediate(() => cb(null, this.option));
  }
}

var y = pify(x);

// With delegate accessors you could do:
y.option = 'bar';
y.method().then(...);

// Without delegate accessors you need to remember to set on x:
x.option = 'bar';
y.method().then(...);

I removed that in the second commit (code is a lot smaller / cleaner without the feature)

@@ -1,8 +1,8 @@
'use strict';

var processFn = function (fn, P, opts) {
var processFn = function (fn, P, opts, bind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jamestalmage Is there a reason not to use opts.bind?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. It caused the optimization test to fail (not sure why, but the optimization killers blog post does mention something about fn.bind().

Also, we are already calling fn.apply(someObj, args) anyways. Adding a layer of indirection where this doesn't end up being someObj seems confusing.

@sindresorhus
Copy link
Owner

@schnittstabil @SamVerschueren What do you think about handling getter/setters too 32e5fe7? It kinda makes sense to me.

@sindresorhus sindresorhus changed the title bind option. add bind option Jun 18, 2016
@sindresorhus
Copy link
Owner

const obj = {
x: 'foo',
y: function (cb) {
setImmediate(() => cb(null, this.x));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it's preferred, but setImmediate will pass on any extra arguments, so this could be written like this:

setImmediate(cb, null, this.x);

@jamestalmage
Copy link
Author

I can update the PR, but do we want the property accessors when binding, or not?

@sindresorhus
Copy link
Owner

@jamestalmage I'm not sure. Are there any potential downsides to it? What does bluebird.promisify do? I'm just afraid of it having some pitfalls and requiring yet another major release at some point later.

@jamestalmage
Copy link
Author

I don't think bluebird binds anything but methods

@schnittstabil
Copy link
Contributor

In my personal vision, pify(obj) will return an ECMAScript6 Proxy object when available, forwarding almost all operations to obj, and promisifying only some functions.

Based on this idea:

  1. pify(obj).foo = 42; would modify obj
  2. binding to an other object than obj would be out of scope of pify

Therefore I don't see any downsides providing 1.

@jamestalmage
Copy link
Author

@schnittstabil - That's basically what the first commit does (and the second commit undoes). It's not a complete proxy object (the property has to exist at the time of promisification).

@schnittstabil
Copy link
Contributor

@jamestalmage I wanted to show, that I don't believe, that your suggestion have to be reverted in the future.
Despite this, Object.keys only returns own properties, thus 32e5fe7 doesn't work with inheritance!?

@jamestalmage
Copy link
Author

Despite this, Object.keys only returns own properties, thus 32e5fe7 doesn't work with inheritance!?

Yeah. I had that thought recently and forgot to mention it. If everybody is 👍 on the feature, I will revert the second commit and just do all enumerable properties (or do we need non enumerable too?)

@sindresorhus
Copy link
Owner

If everybody is 👍 on the feature, I will revert the second commit and just do all enumerable properties

👍

(or do we need non enumerable too?)

Nah. They're non-enumerable for a reason.

@sindresorhus
Copy link
Owner

In my personal vision, pify(obj) will return an ECMAScript6 Proxy object when available, forwarding almost all operations to obj, and promisifying only some functions.

@schnittstabil I really like this idea. Mind opening an issue so we can consider doing that in the future?

@jamestalmage
Copy link
Author

Hmmm:

class Foo {
  bar () {}
}

new Foo().propertyIsEnumerable('bar');
// => false

@sindresorhus
Copy link
Owner

@jamestalmage Agh. I forgot that ES2015 class members are non-enumerable. I guess we have to get those too then.

@jdalton jdalton mentioned this pull request Dec 22, 2016
@schnittstabil
Copy link
Contributor

Because of #36 (comment), I believe the best way to deal with this is a dedicated bind library, bind-all seems suitable:

// instead of 
const y = pify(x, {bind: true});

// we can use a bind library
const y = pify(bindAll(x));

I believe the semantics would be more clear. Furthermore, the loss of performance seems to be negligible, because we have to bind all(relevant!?) functions, not only the functions we want to promisify.

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