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

Implement using Proxy #73

Merged
merged 12 commits into from
Feb 14, 2020
Merged

Implement using Proxy #73

merged 12 commits into from
Feb 14, 2020

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Jun 20, 2019

Fixes #29. Fixes #57.

This implementation changes the behavior when the wrapped object is mutated. It will act as a promisified "view" of the underlying object, rather than a promisified clone. Thus, if properties (including methods) change in the underlying object, the promisified view will also be updated. This change is reflected in a change in one of the existing tests.

I also fixed a bug where symbol properties were incorrectly handled, although that's unrelated to using Proxy.

Regarding this issue in #32:

[[Get]] for proxy objects enforces the following invariants: The value reported for a property must be the same as the value of the corresponding target object property if the target object property is a non-writable, non-configurable own data property.

As far as I could tell, addressing this is complicated because you need to get the property descriptor which may be inherited, and there is no native API to do that so you have manually walk the prototype chain.

Where should we document the caveat that pify will not work in this case?


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@sindresorhus
Copy link
Owner

sindresorhus commented Sep 23, 2019

Thanks for working on this 🙌

As far as I could tell, addressing this is complicated because you need to get the property descriptor which may be inherited, and there is no native API to do that so you have manually walk the prototype chain.

Would be nice to just handle this though. Imagine using pify on an object from a package you don't control, then that package makes a property non-writable in a patch release, and suddenly pify is crashing. While we could document it, it's hard for users to protect themselves against it.

See sindresorhus/on-change#16 for how it was handled on another package of mine.

target object property is a non-writable, non-configurable own data property.

Doesn't the "own" word imply it doesn't apply when inherited?

test.js Outdated
const obj = {[sym]: cb => setImmediate(cb)};
const pified = m(obj);
await pified[sym]();
t.pass();
Copy link
Owner

Choose a reason for hiding this comment

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

Use t.notThrows instead.

@sindresorhus
Copy link
Owner

I think this also fixes:

Can you add some more tests?

@frangio
Copy link
Contributor Author

frangio commented Sep 27, 2019

Doesn't the "own" word imply it doesn't apply when inherited?

Seems so! I totally missed that.

@frangio
Copy link
Contributor Author

frangio commented Sep 30, 2019

I think #63 was fixed in #69. However, it's good that you brough it up because this PR breaks bind when pify is used on a function, because it's also promisifying the function's methods. So pify(fn).bind is actually a promisified version of bind, and pify(fn).bind(obj) is not a function but a promise, and throws an error if it's called as a function.

To fix this I can add an additional condition that we will not promisify a property if its value is taken from Function.prototype. What do you think about this? It feels ad-hoc but since the library is meant to be used with functions in this way, I think it's justified.


#48 is fixed-ish. The problem is that a method called through the pify proxy will receive as this the proxy and not the underlying object, and this will likely break the object because internally it doesn't expect to have promisified methods. I think this can be fixed by switching the proxy for its target in apply, but it has to be done carefully to not break bind. I will look into this.

@sindresorhus
Copy link
Owner

To fix this I can add an additional condition that we will not promisify a property if its value is taken from Function.prototype. What do you think about this? It feels ad-hoc but since the library is meant to be used with functions in this way, I think it's justified.

👍

I think this can be fixed by switching the proxy for its target in apply, but it has to be done carefully to not break bind. I will look into this.

👍

@frangio
Copy link
Contributor Author

frangio commented Nov 17, 2019

@sindresorhus Can you please clarify what tests you’d like me to add?

@frangio
Copy link
Contributor Author

frangio commented Dec 16, 2019

I've tackled the remaining issues mentioned, and added tests for those things. I feel that the existing test suite is quite comprehensive. If you have concrete tests you would like me to add let me know though.

@sindresorhus sindresorhus merged commit 57e9a04 into sindresorhus:master Feb 14, 2020
@sindresorhus
Copy link
Owner

Looks good now. Great work 👌

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.

Promisifying all methods of a ES6 Class instance doesn't seem to work Return ES2015 Proxy object
2 participants