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

docs: add wrappedMethod line #2197

Merged
merged 2 commits into from Jan 26, 2020
Merged

docs: add wrappedMethod line #2197

merged 2 commits into from Jan 26, 2020

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Jan 12, 2020

Fixes #2182

This is a tiny one but it means we can unblock @NoamDev over in DefinitelyTyped/DefinitelyTyped#41219.

There was a typo'd header too i fixed in there. If you want any more info adding to it, let me know.

cc @fatso83

@NoamDev
Copy link

NoamDev commented Jan 12, 2020

from my testings, stub.wrappedMethod will be undefined when stubbing a property
example for stubbing getter/setter
https://runkit.com/embed/0u8nznkm2nu9
example for stubbing raw property
https://runkit.com/embed/mzqczaq4g5xt

@NoamDev
Copy link

NoamDev commented Jan 12, 2020

In addition wrappedMethod is available for spies too.
However, it seems incomplete when you try it on properties.
Edit: I extracted the broblem to a new issue: #2198

@43081j
Copy link
Contributor Author

43081j commented Jan 12, 2020

yep seems that stubbing an accessor doesn't result in wrapMethod being called so we have no wrappedMethod set.

lets see what @fatso83 thinks in case it should have one.

@NoamDev
Copy link

NoamDev commented Jan 12, 2020 via email

@fatso83
Copy link
Contributor

fatso83 commented Jan 14, 2020

Thank you for your contribution!
Unfortunately, using this field is not a feature we support, hence the lack of documentation. This is an internal detail. If we relied on this for the public interface we would have covered it with tests. Alas:

$ ag -l wrappedMethod test/ | wc -l
0

There are 0 lines of test code covering that field, so I am not going to be the one giving a 👍 for documenting that feature. @mroderick might remember more about the history of this internal property, but from what I can gather, this is part of some internal bookkeeping machinery for spies and stubs.

$ ag -l wrappedMethod lib/
lib/sinon/behavior.js
lib/sinon/util/core/wrap-method.js
lib/sinon/mock.js

That being said, I can understand if this field has some value, but I would rather suggest creating an explicit extension of the existing API with a new method that has some clearly defined behavior, preferably also working for getters and setters. We might also possibly hide (by making the fields non-enumerable) the internal fields somehow.

@fatso83 fatso83 closed this Jan 14, 2020
@fatso83 fatso83 reopened this Jan 14, 2020
@fatso83
Copy link
Contributor

fatso83 commented Jan 14, 2020

Reopening, as I read #988, which kind of implies that we have explicitly added this field as a feature. I am not totally sure about how to go about this, though, for the reasons mentioned in the previous comment. At least I think we need to cover this feature with some tests documenting how it works, before adding it to the docs.

@43081j
Copy link
Contributor Author

43081j commented Jan 14, 2020

yep i made this PR because i had seen #988 which claimed it was exposed on purpose, and because i saw that we had a blocked PR over on definitelytyped for the same thing.

you're definitely right it should probably be a method which works for accessors, too. but in its current state, a decision should be made really.

im happy to add tests, but are we certain it should be part of the public api before i do?

Copy link
Member

@mantoni mantoni left a comment

Choose a reason for hiding this comment

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

I'm fine with it being exposed and being an official part of the API. Sinon will always have to keep a reference to the original function. I can't see a good reason to hide it.

@43081j
Copy link
Contributor Author

43081j commented Jan 26, 2020

here's some tests so we can get this merged hopefully

Copy link
Member

@mroderick mroderick left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution to Sinon. Improving the API documentation is hugely valuable to the adoption of a project like Sinon 🙇

@mroderick mroderick merged commit f8b8606 into sinonjs:master Jan 26, 2020
@43081j 43081j deleted the wrapped-docs branch January 26, 2020 21:06
@NoamDev
Copy link

NoamDev commented Jan 27, 2020

I believe that if it was declared as on official feature, then we should solve #2198

@mroderick
Copy link
Member

I believe that if it was declared as on official feature, then we should solve #2198

It's in the documentation now, so it's official!

If it gets removed in future version(s), then that will be a breaking change and a new MAJOR version.

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.

Document stub.wrappedMethod property
5 participants