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

Throw error when trying to stub non-configurable or non-writable properties #2377

Closed
fatso83 opened this issue May 25, 2021 · 1 comment
Closed

Comments

@fatso83
Copy link
Contributor

fatso83 commented May 25, 2021

Is your feature request related to a problem? Please describe.
Today we silently ignore stubbing issues, such as trying to use sinon.stub(obj, 'prop'), when obj.prop is a synthetic getter. From a user perspective, it is probably more user friendly to be explicitly told immediately that this Object.descriptor is non-configurable (perhaps even with a link to a Sinon doc page!) and cannot be stubbed, than the current behavior.

This situation applies to a lot of different use cases, especially the ones involving transpilation using Webpack or TypeScript. For instance,lthough the #2319 PR talks about TypeScript and ECMAScript modules, this all basically boils down to:

(...) some way of notifying the user explicitly that this code is not stubbable

Describe the solution you'd like
We should throw an error when encountering property descriptors that are not stubbable. While we have silently ignored these in the past, it is not like we don't already do this in our code base. We already throw errors if you try to stub a true ESM:
https://github.com/sinonjs/sinon/blob/master/test/es2015/module-support-assessment-test.es6#L53-L68

From MDN

Property descriptors present in objects come in two main flavors: data descriptors and accessor descriptors. A data descriptor is a property that has a value, which may or may not be writable. An accessor descriptor is a property described by a getter-setter pair of functions. A descriptor must be one of these two flavors; it cannot be both.

So throw in these cases

if(!descriptor.configurable && !descriptor.writable)
    throw new TypeError(`Descriptor for property ${prop} is non-configurable and non-writable: ${descriptor}`);
if( (descriptor.get || descriptor.set) && !descriptor.configurable)
    throw new TypeError(`Descriptor for accessor property ${prop} is non-configurable: ${descriptor}`);
if( isDataDescriptor(descriptor) && !descriptor.writable)
    throw new TypeError(`Descriptor for data property ${prop} is non-writable: ${descriptor}`);

isDataDescriptor must return something based on this:

If a descriptor has neither of value, writable, get and set keys, it is treated as a data descriptor. If a descriptor has both [value or writable] and [get or set] keys, an exception is thrown.

Describe alternatives you've considered
Printing a one-time warning?

Additional context
#2319, #2238 and others

@sdotson
Copy link
Contributor

sdotson commented Dec 23, 2021

I went ahead and took a shot at this. Let me know if there are any requested changes. #2417

fatso83 pushed a commit that referenced this issue Dec 24, 2021
…table properties (#2417)

Fixes issue #2377 by throwing an error when trying to stub non-configurable or non-writable properties
@fatso83 fatso83 closed this as completed Dec 24, 2021
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

4 participants