Sinon: Support constructor functions and ES6 classes in spies and stubs

[bisubus]

Currently spies and stubs don't call original or stub function with new by design - even if they were called with new. The suggestion is to add this behaviour to them.

To my knowledge, some of the cases could be easily treated with createStubInstance, while in other cases it will be difficult. Since createStubInstance creates an instance, a wrapper function should be created manually, and if there should be static methods, they should be created manually too. If test case presumes that wrapper function should be called with or without new, this should be coded manually too.

I believe that doing this in sinon.spy and sinon.stub would result in more consistent behaviour.

• Sinon version : 2.0.0-pre.5
• Environment : Node.js

What did you expect to happen?

I would expect that something like this will happen inside a spy/stub:

Code

var newTarget;

try {
  newTarget = eval('new.target');
} catch (err) {
  if (err instanceOf SyntaxError)
    newTarget = this && (typeof this === 'object' || typeof this === 'function')
      && this.constructor;
}

if (newTarget) {
  return new originalFn.bind(this, ...args)
} else {
  return originalFn(...args);
}

What actually happens

Original spied/stubbed classes and constructor function aren't called with new. This includes ES6 classes that were transpiled with Babel and have a safeguard against being called directly. This results in error:

Class constructor MockedBar cannot be invoked without 'new'

How to reproduce

const foo = { Bar: class Bar {
  static factory() { return new Bar }
} };

sinon.stub(foo, 'Bar', class MockedBar {
  static factory() { return new MockedBar }
} )

foo.Bar.factory()
new foo.Bar()

[fatso83]

I have a bit of trouble understanding what you are trying to do with the actual code from the top of my head, but I do recognize the heading, which has been covered extensively before.

Could you please read the the comment thread in 831 and see if it covers the info on what you are trying to do? Alternatively this or this.

I might be totally off on what you are on about, but it bears mentioning that ES6 is just syntactic sugar and _there is nothing you cannot do in ES6 regarding classes and constructors that wasn't possible with ES5_. So hoping this issue just goes away by itself 🙈

😸

[bisubus]

@fatso83 Sure, I've already read related issues prior to posting this one.

The thing that differs ES6 classes from ES5 constructor functions is a safeguard that prevents them from being used like var bar = Object.create(Bar.prototype); Bar.call(bar).

In the example above foo.Bar method is a class which is called somewhere else like new foo.Bar(), but it cannot be stubbed with another class (only with a function, like you've shown here), and more importantly, it cannot be spied at all (because spy proxy function doesn't call the original with new).

[fatso83]

@bisubus thank you for the details. I was not aware that the spec actually mandates the interpreter to throw an error when not calling the constructor with new, since the transpiled code is working fine. Since this feature is currently not possible to support in ES6/ES2015 environments using ES5-only syntax, we will need to do something à la

1. feature detect that we are running in a ES2015 environment
2. Use the Proxy feature to create a stub that won't throw

We definitively need to support this, so feel free to hack out a PR that patches createStubInstance.

[bisubus]

@fatso83 Babel tries to follow the specs strictly (TS doesn't, Traceur too IIRC), so this is applicable to ES5 as well. It is possible to detect native classes with regexps, but not Babel classes, at least in minified code.

Did you mean 'stub' when you said 'createStubInstance'? It seems to me that createStubInstance is fine, at least sinon.createStubInstance(NativeClass) creates an instance with new and doesn't throw.

I think there's no real need for Proxy in this case, the same thing could be done in ES5 with

var NativeClassWrapper = Object.setPrototypeOf(function () {
  return new (NativeClass.bind.apply(NativeClass, [this].concat(Array.from(arguments))))
}, NativeClass)
NativeClassWrapper()

But wouldn't this be an XY problem? Accidental new can be harmful too but is harder to debug. It looks like a job for spies and stubs (especially for spies) to detect if they were called with new or not and propagate the behaviour to original function/class.

Do you have ideas or comments on incorporating newTarget detection shown in the OP into spies/stubs? The real thing has more checks around new.target, but detection method for ES6 with ES5 fallback looks like that.

[fatso83]

Just a minor thing.

The example above turned out to (very slightly) incorrect. I was playing around with it and noticed it wasn't actually doing what you said. Factory methods are static by nature, but you omitted the static keyword in your definition, which means that foo.Bar.factory() will never work - sinon or no sinon, as it would be just a method on the prototype otherwise.

I took the liberty of adding the static keyword to make the example runnable.

I/someone will need to come back to you on the other points you raised.

P.S. The OP needs to be rephrased a bit to make it clearer that this is a feature request for more convenience, rather than a bug (unless I am misunderstanding and it is both things).

[bisubus]

@fatso83 Thanks, of course, this was a mistake.

I've updated the OP. Since this behaviour existed in spies and stub by design, I guess this is feature request.

[ProLoser]

I think the spy should detect that it was called with new and if it was, should call through with new too. It shouldn't matter what you're spying on, the spy should call through the same it way it was called.

Can't we just move the thisCall.calledWithNew() check before the func.apply() and if it's true, use the new keyword on the call through?

This feels like it would fix all the problems we're having.

[fatso83]

Fix out in version 4.1.3, courtesy of @ProLoser

[fatso83]

I was a bit premature in closing this, I see. #1626 deals explicitly with the spying case, not the stubbing case. Reopening.

After seeing the code for the spy case, I am reconsidering what I have said earlier about constructor stubbing. In my mind I didn't really see that it was possible, as stubbing usually means replacing the function, and if you replace the function representing the class then what would be left of the class?

Then after seeing the code @ProLoser supplied for spying on the constructor it dawned upon me that I needed to tilt the perspective over to what the user wants: I would guess what a user _wants_ is to invoke a Sinon method that would enable her to:

1. create objects where the constructor has been replaced (for instance to avoid calling some collaborator)
2. still keep all the other methods
3. pass the instanceof test
4. be able to assert on normal stubbing methods and props

I must attest to not understanding the expected code, @bisubus, but is the sinon.stubConstructor in the following code what you basically would want? It works and fulfills the bits above.

const sinon = require('./')

const foo = { Bar: class Bar {
    static factory() { return new Bar }

    constructor(){
        this.baz = 1;
    }

    getBaz() { return this.baz}
} };

sinon.stubConstructor = function(constructor, fake) {
    return sinon.spy((...args) => {                            // alternatively: sinon.stub().callsFake(..)
        var i = Object.create(constructor.prototype);
        fake.apply(i, args);
        return i;
    })
}

var stubbedConstructor = sinon.stubConstructor(foo.Bar, function() {
    this.baz = 42;
});

console.log('Tests on using the stubbed constructor')
var myBar = new stubbedConstructor();
console.log('instanceof', myBar instanceof foo.Bar); // ==> true
console.log('this.getBaz()', myBar.getBaz()); //  ==> 42
new stubbedConstructor();
new stubbedConstructor();
console.log('Call count', stubbedConstructor.callCount); //  ==> 3

Not totally sure about the sinon.stub().callsFake(..) vs sinon.spy(...) bits, but I can't see any immediate things one could do with a true stub as it would not make sense to return anything else than the instance.

If this is what people wants, then I could supply the PR, but the name is just a suggestion. What do you think, @mroderick?

[ProLoser]

To add my own thoughts; I was trying to fix a bug where calling an es6 class constructor without 'new' would throw. After looking at the source I realized the proxy was not calling the original in the exact same way the proxy gets called in the test and to me created a discrepancy. I want my original to be run identically to how I ran the proxy.

[bisubus]

@fatso83 More or less. The use of sinon.stub in stubConstructor seems more reasonable. stubConstructor should also contain newTarget part from OP, in order to conform to the expectations:

new stubbedConstructor(); // passes
stubbedConstructor(); // fails

I guess this still doesn't solve the problem completely; stubbedConstructor won't work correctly with callThrough, will it?

[fatso83]

No, it wouldn't. At least without much custom hacking, reproducing existing code, as we can't really use the stub for much, hence using a spy. This would basically cover the 80/20 case, not getting all details right, but still addressing the issues of the most common needs.

[stale[bot]]

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.