Simplify/make props definition consistent

[fatso83]

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 use false if that attribute was already set to false. Why does that make a difference? Well, as long as the prop is writable Object.defineProperty will only throw if configurable is false AND any of the other fields change their value. So we can do Object.defineProperty(obj, prop, {value:42}) on a non-configurable prop, as the other fields will be left alone.

Why do we tread enumerable differently than configurable? Given the above, reasoning, why do we always set enumerable to true? Should it now be subjected to the same handling as configurable? Yes it should have been treated in the same manner and in fact, trying to do stub.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:

var getObjectDescriptor = require('./get-object-descriptor');
var defaultPropValues = Object.freeze({configurable: true, enumerable: true, writable: true});

// reuse existing values, if they exist
function defineProperty(obj, prop, newAttributes){
    var originalDescriptor = getObjectDescriptor(obj, prop);
    var descriptor = Object.assign({}, defaultPropValues, originalDescriptor || {}, newAttributes);
    Object.defineProperty(object, prop, descriptor);
}

The reason we need to merge in defaultProps is that all of the listed ones are false by default when using Object.defineProperty while we usually want them to be true (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.

[victoriatomzik]

Can we close it?

[fatso83]

@victoriatomzik No work has been done on this, so no. Do you want to have a go? It is quite clearly detailed and ready 👍