Chai: Sanity check for assertion instanceOf

[fgarcia]

The current implementation for the instanceOf assert does not check if the given constructor is defined

https://github.com/chaijs/chai/blob/104a6b7db9e02d24cd296fbeddc3f55907e8fda0/lib/chai/core/assertions.js#L989

There is a common error I hit several times where an imported variable stops working. I use that variable to assert the instance of an object, like:

import { Pathname } from 'core'
....
expect(x).to.be.instanceOf(Pathname)

When the import fails (silently) with an undefined value, the error thrown by the assertion shows a normal Javascript error

TypeError: Cannot read property 'name' of undefined

It would be great getting something more meaningful like

ChaiError: instanceOf() needs a constructor. undefined value given

NOTE:

I've realized there is no such thing as ChaiError error subtype. I am not sure how far the current code base goes to improve bad use cases

[vieiralucas]

Hello @fgarcia thanks for your issue.

IMHO this is a chai problem, I agree with you that this should provide a more meaningful error message.
Since we move the getName utility to it's own module (chaijs/get-func-name), currently on 4.0.0-canary.1 this error would be throw from here.

So, IMHO:

• get-func-name should throw its own exception if undefined is given. Maybe we should open an issue there
• instanceOf should do something similar to above

It should be very easy to do this, we just need to add a check here

Am I missing something here? Do someone disagree?

[lucasfcosta]

Hi @fgarcia, thanks for your issue! 😄

Well, I totally agree with @vieiralucas that we should fix this.

However, I think throwing an error is something that should be done within Chai's core.

IMO the whole point of having a separate module for get-func-name is making it modular enough to be used by anyone wanting to, including us. By throwing an error there we would be coupling what we want for Chai with the module's own purpose.

I think that it would be more adequate to just add a check to getFunctionName to avoid it throwing a TypeError and then just return undefined if undefined was passed to it. Then we could just add our own specific logic here at Chai to throw a custom AssertionError, since this is useful for us only, after all, not everyone using get-func-name will expect it to throw this kind of Error.

Feel free to share your thoughts :)

[vieiralucas]

I didnt mean to change get-func-name to throw a chai specific custom error.
I was just saying that it should not be exploding trying to access an property on undefined.
I guess returning undefined is good too, but as I said we should discuss it over there.
:smile:

[shvaikalesh]

Agreed that we should validate what is getting passed to instanceOf assertion.

However, with ES6's @@hasInstance that is not trivial task to do. Second operand of instanceof is allowed to be either an object with callable @@hasInstance or a function (not necessary with [[Construct]]) with prototype object. First operand can be of any type, primitives included.

[meeber]

Fixed via #899