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

Consider supporting prototype methods #14

Closed
kevinoid opened this issue Dec 30, 2015 · 5 comments
Closed

Consider supporting prototype methods #14

kevinoid opened this issue Dec 30, 2015 · 5 comments

Comments

@kevinoid
Copy link
Contributor

Any chance you would be willing to consider supporting pify-ing the prototypally-inherited methods of passed objects?

The main issue that I'm trying to solve is that code such as:

var lovebug = pify(new Car("Herbie"));

works if the Car constructor sets/copies/binds methods to this, but not if the methods are inherited via prototype. So, unless I've missed something, pify users must either depend on the implementation of Car or roll their own method iteration wrapper for pify (and/or pify the prototype, then pify any methods set by the constructor).

If pify-ing inherited methods sounds agreeable but you'd need a PR to consider, I can work one up. I would envision the methods being copied from the prototype to the new target object if a boolean option is set, but alternatives would be fine with me.

Thanks for considering,
Kevin

@sindresorhus
Copy link
Owner

Not something I personally need, but I can see it being useful for many, so PR welcome :) Should be a boolean option that is false by default.

You might find inspiration in the Bluebird implementation https://github.com/petkaantonov/bluebird/blob/master/src/promisify.js

@kevinoid
Copy link
Contributor Author

Great, thanks! I'll see what I can do.

@schnittstabil
Copy link
Contributor

@kevinoid I assume we only consider Enumerable and Inherited Enumerable properties. In that case I think it is sufficient to choose Object.keys or for…in depending on your option at index.js#L59.

@kevinoid
Copy link
Contributor Author

@schnittstabil I agree, that sounds like the right behavior and implementation to me. Thanks.

Feel free to do the PR if you are strongly motivated, I'm not picky about it. Otherwise I should have some time to hammer it out later today.

@schnittstabil
Copy link
Contributor

Sorry @kevinoid, I do neither see the need for that option nor have much time to implement it.

kevinoid added a commit to kevinoid/pify that referenced this issue Jan 8, 2016
This makes it possible to transform objects which are instances of a
class without knowing which methods are present on the instance object
and which are inherited through the prototype chain.  Include/Exclude is
handled the same as for non-inherited methods.

In addition to transforming inherited methods, when the inherited option
is enabled the result object is created with the prototype of the
original object so that it behaves the same as the original object for
instanceof checks.

Fixes sindresorhus#14

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
kevinoid added a commit to kevinoid/pify that referenced this issue Jan 27, 2016
This makes it possible to transform objects which are instances of a
class without knowing which methods are present on the instance object
and which are inherited through the prototype chain.  Include/Exclude is
handled the same as for non-inherited methods.

In addition to transforming inherited methods, when the inherited option
is enabled the result object is created with the prototype of the
original object so that it behaves the same as the original object for
instanceof checks.

Fixes sindresorhus#14

Changes in v2:
- Default-initialize ret to an empty object.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
kevinoid added a commit to kevinoid/pify that referenced this issue Jan 27, 2016
This makes it possible to transform objects which are instances of a
class without knowing which methods are present on the instance object
and which are inherited through the prototype chain.  Include/Exclude is
handled the same as for non-inherited methods.

In addition to transforming inherited methods, when the inherited option
is enabled the result object is created with the prototype of the
original object so that it behaves the same as the original object for
instanceof checks.

Fixes sindresorhus#14

Changes in v2:
- Default-initialize ret to an empty object.

Changes in v3:
- Replace reduction over keys array with a for-in loop.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
kevinoid added a commit to kevinoid/pify that referenced this issue Jan 28, 2017
This makes it possible to transform objects which are instances of a
class without knowing which methods are present on the instance object
and which are inherited through the prototype chain.  Include/Exclude is
handled the same as for non-inherited methods.

In addition to transforming inherited methods, when the inherited option
is enabled the result object is created with the prototype of the
original object so that it behaves the same as the original object for
instanceof checks.

Fixes sindresorhus#14

Changes in v2:
- Default-initialize ret to an empty object.

Changes in v3:
- Replace reduction over keys array with a for-in loop.

Changes in v4:
- Rebase on to latest master (ES2015, ava changes, style changes).

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants