-
-
Notifications
You must be signed in to change notification settings - Fork 772
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
Simplify/make props definition consistent #2238
Comments
2 tasks
Can we close it? |
fatso83
added
hacktoberfest-accepted
hacktoberfest
and removed
hacktoberfest-accepted
labels
Oct 8, 2020
@victoriatomzik No work has been done on this, so no. Do you want to have a go? It is quite clearly detailed and ready 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
How we handle defining props (basically how we replace props with fakes or stubs) is handled a bit nilly-willy, causing random bugs. An analysis in #2237 showed that any special handling only needs to happen when two things happen: the prop is non-configurable and it is owned by the object.
This issue exists to remind us that we should devise a way to handle this and then hunt down which of the ten-or-so places that are applicable.
This is a section from #2237 that contains details about this issue. Refer to that case for background info:
... there are several things I found out while researching this that needs fixing.
Why do we not always set
configurable: true
?Looking at the git log I found #1456 and #1462 which basically changed
true
to usefalse
if that attribute was already set tofalse
. Why does that make a difference? Well, as long as the prop is writableObject.defineProperty
will only throw ifconfigurable
isfalse
AND any of the other fields change their value. So we can doObject.defineProperty(obj, prop, {value:42})
on a non-configurable prop, as the other fields will be left alone.Why do we tread
enumerable
differently thanconfigurable
?Given the above, reasoning, why do we always set
enumerable
to true? Should it now be subjected to the same handling asconfigurable
? Yes it should have been treated in the same manner and in fact, trying to dostub.value(42)
on a prop that is neither configurable nor enumerable will throw in Sinon today. As should be apparent from the above section, this does not need to happen! We only need to keep properties the same, if they exist, otherwise, use sensible (permissive) default values.Given that, I think we need to create a separate issue to cover that. A basic fix is to make our own
defineProps
that does this:The reason we need to merge in defaultProps is that all of the listed ones are
false
by default when usingObject.defineProperty
while we usually want them to betrue
(like they are in normal assignment operations).The suggested fix is just a quick suggestion. Another might be:
Always create props with the default permissive settings, unless
propDefinition.isOwn && propDefinition.configurable === false
, in which case you need to merge them as done in the suggestion.The text was updated successfully, but these errors were encountered: