Sinon: Stub get/set on property descriptors with the new API (after deprecating stubDescriptor)

[lucasfcosta]

Hi friends, as I have commented here, when the stubDescriptor gets deprecated we will not be able to stub getters and setters on property descriptors anymore since the function it calls will not be available anymore (the one that allows stubbing descriptors).

I'd either suggest we do not deprecate it since IMO its API is already good or start using get and set methods chained with the stub. Like this:

// This will not be available anymore
var stub = createStub(obj, "prop", {
  get: function () {
    return 43;
  }
});

// We would write this instead:
var stub = createStub(obj, "prop").get(function () {
  return 43;
});

The same goes for setters.

What do you think?

[fatso83]

Hi, @lucasfcosta, sorry for leaving this hanging for so long. It's a crucial API design bit I think we should decide on what to do with ASAP. I at least forgot this bit when looking at #1239 since I so seldomly stub properties. Hopefully we can decide before the release hackathon we are planning, or what @mroderick and @mantoni? Hopefully we have decided on most bigger decisions before that so we can focus on the lower hanging fruits.

I think your proposal looks good. Are you thinking it should be chainable, or what do you intend to do with properties that have both getters and setters?

[lucasfcosta]

@fatso83 No problem, sometimes life gets really busy and we have to worry about things other than open source. I too haven't had much time available to send more PRs to Sinon in the last month as I was doing on last year's holidays, I'm looking forward to contributing more next month. Thank you and all the team for your dedication to the project!

---

Regarding the proposal:
I think that both get and set methods should return the stub itself so we can have both one-liners and we will still be able to write get and set in a single statement. I think this is the optimal decision for this case because there is not anything else that is interesting to do with getter/setter functions (at least I can't remember any).

Also, I think that it is really important for this to be this way because of what I've read in #817. Given that issue and the user's comments we've got there, I think this is what they would expect Sinon to do.

This is an example of what this would allow us to do:

// Since `get` returns the stub itself we don't need to assign in a line and call `get` in another
var stub = createStub(obj, 'prop').get(function () {
  return 43;
});

// We can also chain `get/set` because they will run on the stub returned by each other
// In this case `stub` is also the original stub object, as in the example above
var stub = createStub(obj, 'prop').get(function() { return 'bla' }).set(function() { return 'foo' });

I can start implementing this as soon as the team approves this proposal. Also, there's no need to rush, feel free to do things when you have time.

PS: I'd love to participate on this hackathon.

[fatso83]

OK, going at this today ...

[lucasfcosta]

Thanks @fatso83!
I can review your work when you finish or help you with anything you need.
Let me know if you need any help.

Also, here is what I've thought about when wandering through the codebase to see how this could be implemented:

1. First we'd need to remove this check right here in order to be able to stub properties
2. Currently we store the name of the property being wrapped into method.displayName as you can see in this line, but since it only gets added to the stubbed method itself we can't access it in the stub itself so we will need to store the name of the property being stubbed in order to change its property descriptor later.
3. Now we must add a new behavior and call Object.defineProperty passing the stub to it and the name of the property being stubbed (which has been stored at method.displayName. The property descriptor passed as the third argument must then assign the function passed to the stub's get method to the get property of the property descriptor.

I think this would be enough to implement this feature. For setters we just need to apply the same logic but use the set property of the property descriptor (obviously).

It's also important to notice that if there's a namespace clash with any existing properties on the stub object, it won't be possible to call them anymore.

This is a very basic explanation of what I've thought when I was thinking about how to implement this feature so I might have not noticed some other details, but I think it's pretty much it. Let me know if you need anything.

[lucasfcosta]

Hi friends, I think we can close this one.

[lucasfcosta]

Hi friends, I think we can close this one.