feat: throw when trying to use .value() on a method#2631
feat: throw when trying to use .value() on a method#2631
Conversation
This prevents an error when running puppeteer/chrome on latest Ubuntu See: - https://github.com/sinonjs/sinon/actions/runs/12502499977/job/34881594651?pr=2631 - puppeteer/puppeteer#13365
|
@mantoni the errors seen in 
If you're seeing similar errors in Mochify, you might want to stick to |
|
|
||
| if (propertyIsMethod) { | ||
| throw new Error( | ||
| `${rootStub.propName} is a function, not a getter or value. Use .returns() instead of .value()`, |
There was a problem hiding this comment.
Are we supposed to allow setting .value() on a getter property? Seems erronous.
There was a problem hiding this comment.
I wasn't sure either ... to me it seemed more reasonable to use .returns() rather than .value() on a getter. I'm happy to change it to do that
There was a problem hiding this comment.
I think we should just do the simplest thing: prevent using value() on a method. So "${rootStub.propName} is a function and should not be overridden by .value(). Use .returns() when creating a function that returns a value.
There is little reason to add synthetic property getters and setters to the mix, as it just adds more noise/possible confusion, IMHO.
| it("allows stubbing getters", function () { | ||
| const y = { | ||
| get foo() { | ||
| return "bar"; | ||
| }, | ||
| }; | ||
| refute.exception(function () { | ||
| createStub(y, "foo").value("bar"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The original issue (#2629) does not deal with this issue, so this seems more like we are documenting existing (non-intentional) behavior? Dan does talk about getters, but that is the conventional Java-bean like getters, which are methods.
There was a problem hiding this comment.
If we change it like in #2631 (comment), then this test will obviously also need to be changed
There was a problem hiding this comment.
Stalled for a year. Do whatever needs to be done and get a new breaking change for 2026 🎉?
2 participants
Purpose (TL;DR) - mandatory
This is a solution for #2629
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyDescriptor#get for the
.getproperty on the property descriptor.How to verify - mandatory
npm installnpm testChecklist for author
npm run lintpasses