Conditional support for replacing props not defined by the object

[fatso83]

Is your feature request related to a problem? Please describe.
In #1557 we removed support for stubbing undefined properties, aligning the behavior with how sandbox stubbing worked. Later on we have made the sinon methods use a default sandbox to further align how the default methods and sandboxes worked.

Still, as the thread in #1537 show, this remains a sorely missed feature for some, and while we had good reasons for removing it, maybe we could appease the ones wanting it by making it an opt-in? It remains a quite useful feature in some cases.

Describe the solution you'd like
I suggest adding a config field to the sandbox to allow defining fields (fakes) that are not originally defined by the objects we insert the fake into. Something like sinon.createSandbox({allowUndefined: true})? (Totally not sure about the actual name of the prop, though ... allowNonOwn?)

Given

var obj = {a: 1};

it should allow

sinon.stub(obj, 'b').value(2); // not defined anywhere on the prototype chain
sinon.replace(obj, 'c', sinon.fake()); 

Related to this, but really a different case, is props on the prototype chain. As explained in the "Solution" paragraph in #2193 we deal with this differently today, depending on whether we use spies or fakes:

var parent = { foo: function() {} };
var child = Object.create(parent):

// disallowed
sinon.stub(child, 'foo');
// allowed
sinon.replace(child, 'foo', sinon.fake());

If we simplified the "undefined" and "props on an ancestor" problems into a single issue: props not defined by the object whose fields are being stubbed, we could align this behavior for our spies, stubs and fakes under a default non-permissive line and have an opt-in solution for those who want it. By default, you would then only be allowed to replace fields owned/defined by the object.

Having the opt-in for stubs and spies would not be a breaking change, but as the sinon.replace method today allows stubbing defined props owned by an ancestor it would be a breaking change to disallow this by default.

Describe alternatives you've considered
Two flags instead of one: allowUndefinedProps (props are not present anywhere on the prototype chain) and allowShadowingFieldsOnPrototype (to stub fields defined by ancestors). I don't really see the benefit, though.

[mantoni]

As I understand this, sinon.replace throws on undefined properties to highlight an issue in case the replaced property was removed or there is a typo while writing a test. I quite like this and I agree we should keep it this way.

As I understand the argument for allowing to stub undefined properties, it wouuld be nice to add a property to an object for the test run and let the sandbox remove it reliably. I would like to do this myself sometimes and keep fiddeling around it.

With this in mind, I would like to propose an alternative to the suggested sandbox option here:

sinon.define(child, 'foo', sinon.fake());

I see these advantages:

• This states that I want to define a property "foo" on child.
• It can throw if "foo" is already defined.
• We can have a single sandbox implementation that still throws if sinon.replace is used on undefined properties and also explicitly define undefined properties if needed.

What do you think?

[mroderick]

I agree that it's unfortunate that fakes, spies and stubs behave differently, and that we should address this. Thank you for creating this issue to discuss it 🥇

I suggest adding a config field to the sandbox to ...

I am not a fan of configuration, as that has bitten me in the past. PHP was notoriously difficult to maintain in the past, because the behaviour of functions and APIs changed when you made changes to php.ini.

Having a flag that changes behaviour, means that you can't look at some code and be certain you understand what it does.

I think the sinon.define idea is interesting. How can we explore that further?

[fatso83]

Well, that does not really change anything about the feature disparity (being able to sinon.stub() things on the prototype) and how things work (throws or not). The stubs/spies interface would still not have a way of doing this. Are we fine with this? Is our answer then: "if you want to define fakes on undefined props" you need to use the new fakes implementation - the spies/stubs will remain as is? Instead of a global sandbox flag, we could add an options argument for spy()/stub() to allow shadowing prototype fields.

P.S. The sinon.define thing is fine, btw.

[mantoni]

You could always do sinon.define(obj, prop, sinon.stub()), just like you can do with sinon.replace. It’s not limited to the new fakes API.

[fatso83]

OK, with that in mind, then sinon.define rocks 🚀

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mroderick]

@fatso83 do you agree with the bot closing this issue?

[fatso83]

Not sure. Maybe it should have been recreated as "add a way to define fakes for undefined props", to reflect our discussion. That was a good suggestion, but my original gripe - lack of alignment in behaviour didn't seem to end up in anything. Still thinks it is in unfortunate that we have different ways to handle proto props and/or undefined props, depending on which fake implementation you choose.

[mroderick]

Not sure. Maybe it should have been recreated as "add a way to define fakes for undefined props", to reflect our discussion. That was a good suggestion, but my original gripe - lack of alignment in behaviour didn't seem to end up in anything. Still thinks it is in unfortunate that we have different ways to handle proto props and/or undefined props, depending on which fake implementation you choose.

I agree that sinon.define would be very nice addition. Perhaps that can be a separate issue and then we can keep this issue for figuring out how to align things?

[gnclmorais]

One more vote for this feature and the sinon.define(…)… Maybe also throwing an error if you are trying to define something that exists (just to make sure people use this method in the right circumstances)?