Babel exceptions and assert.throws/assert.doesNotThrow breaks on class-call-check

[silkentrance]

In line https://github.com/nodejs/node/blob/master/lib/assert.js#L273 you will call the expected.call().

     TypeError: Cannot call a class as a function
      at exports.default (node_modules/babel-runtime/helpers/class-call-check.js:5:11)
      at Object.WeirdError (node_modules/common/lib/exceptions.js:1:594)
      at expectedException (assert.js:273:23)
      at _throws (assert.js:310:8)
      at assert.throws (assert.js:319:11)

The mocha test being run is

    it('must throw when doSomething() fails',
    function ()
    {   
        assert.expect(1);
        assert.throws(
        function ()
        {   
            util.doSomething();
        }, WeirdError);
    });

Of course, doSomething() does not yet throw the WeirdError, instead it will throw TypeError.

Now, in expectedException(), WeirdError will be assumed a callable, however, with Babel/es classes, this is no longer true as the new keyword must be used to construct instances of that type.

Would it be possible to add additional expected.prototype tests before trying to call upon the exception class?

E.g. before line https://github.com/nodejs/node/blob/master/lib/assert.js#L273 one would add something like this to avoid expected.call()

else if (Error.isPrototypeOf(expected)) {
    // it is safe to just return false here
    return false;
}

[Trott]

Especially if you're working with new and shiny stuff via Babel, you will be better off using a more robust userland alternative. Unfortunately, the issue you cite here is far from the only shortcoming of the library when dealing with ES6.

The Assert API is locked (although the web site hasn't caught up to that yet, but it will soon) and is unlikely to be improved substantially unless its defects are causing problems with the Node.js project tests. Because of that, I'm going to close this.

I'm not a fan of emoticons in GitHub issue text, but I don't see how I can actually close this without a :-/ and a :-(. So:

:-/

:-(

[silkentrance]

@Trott so I need my custom error check or throws() assertion to overcome this problem. That is fine with me, as I have been working on that anyway. Sad though, that such defects in the software will not be fixed, even though the change is only small and with the least of impact.

Even more so with node now being joined with iojs, again and their overall commitment towards ES6+ and standards compliance.

Of course, a lot of APIs out there use the call class pattern in favor or as a fall back in case the user forgot the new keyword...

[silkentrance]

@Trott please reopen. since node does support harmony classes and classes by default do not support being called without using the new operator I strongly believe this to be a bug in node.

[evanlucas]

isn't that a limitation of the spec?

[Trott]

@evanlucas I think @silkentrance is talking about how this fails:

'use strict';
const assert = require('assert');

const ES6Error = class {
  constructor() {
    this.error('Hello, I am an ES6 clas that can be used as an error.');
  }
};

const functionThatThrows = function () {
  throw new ES6Error();
};

console.log('The next line has an assertion that should pass.');
assert.throws(functionThatThrows, ES6Error.prototype.constructor);

Putting this in test.js and running 5.1.1, I get this:

$ node test.js 
The next line has an assertion that should pass.
/tmp/test.js:5
  constructor() {
             ^

TypeError: Class constructors cannot be invoked without 'new'
    at Object.<anonymous> (/tmp/test.js:5:14)
    at expectedException (assert.js:281:19)
    at Function._throws (assert.js:314:8)
    at Function.assert.throws (assert.js:323:11)
    at Object.<anonymous> (/tmp/test.js:15:8)
    at Module._compile (module.js:425:26)
    at Object.Module._extensions..js (module.js:432:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:313:12)
    at Function.Module.runMain (module.js:457:10)

The question is whether the implementation needs to be fixed so that it works with ES6 classes or whether the documentation needs to be updated to reflect that the second argument to assert.throws() cannot be an ES6 class constructor. Current docs say that it:

can be a constructor, RegExp, or validation function.

That was written before ES6 class constructors were a thing.

@silkentrance As a workaround, you can pass a validation function instead of a constructor.

Based on my sense of the viewpoint of @nodejs/tsc the last time this was discussed there, I suspect the project will opt for the documentation fix. Honestly, I feel like the Assert API was Locked when a doc-only deprecation might have been more in line with (for example) the opinion expressed by some that bugs should not be fixed in the Assert API if they don't affect the Node project's tests. (At least, that was my takeaway. Maybe I'm under the wrong impression.) (And yes, I tagged the TSC group above specifically to see if anyone can clarify if my take is more-or-less right.)

Anyway, re-opening because this should probably get a doc update at an absolute minimum.

[silkentrance]

@Trott thanks, I managed to come over this, though, by implementing the following in my esaver project:

https://github.com/coldrye-es/esaver/blob/master/src/node.es#L45

It will inject a custom check function instead of the error subclass unless the user did not provide a custom check. However, making that a functionality of node's assert would be so much better. Especially since node must expect users to subclass native classes such as Error.

And, please, this should not be fixed by simply documenting a special case but instead by compensating for and expecting any specification limitations.

Subsequently, of course, reopening the module for further development as it clearly does not meet the new requirements.

[Trott]

There's an error in my ES6 class above. When I correct the error, things work as expected. This code works fine:

'use strict';
const assert = require('assert');

const ES6Error = class {
  constructor() {
    this.error = 'Hello, I am an ES6 clas that can be used as an error.'; // this is the line that had an error before
  }
};

const functionThatThrows = function () {
  throw new ES6Error();
};

console.log('The next line has an assertion that should pass.');
assert.throws(functionThatThrows, ES6Error);

Is it remotely possible that there's a similar error in your ES6 class, @silkentrance? If not, can you modify the above code (or create your own) to make a minimal test showing the issue?

[silkentrance]

Well, what I am using is simply

import assert from 'assert';

export class ES6Error extends Error
{
    constructor(message, data)
    {
         super(message);

         this._data = data;
    }

    get data()
    {
        return this.data;
    }
}

assert.throws(
function ()
{
     // we will not throw the ES6Error here as otherwise the bug will not be triggered
     // instead we will simply throw Error so that when testing for instanceof, 
     // assert.throws (expectedException) will then continue with the actuall call to
     // the then expected custom check
     throw new Error('unexpected');
}, ES6Error);

[silkentrance]

@Trott in your functionThatThrows() you should not throw an instance of the expected error as this will suffice the instanceof check in assert/expectedException().

[Trott]

Ah! OK, so this code below should throw an AssertionError but is instead triggering the TypeError.

'use strict';
const assert = require('assert');

const ES6Error = class {
  constructor() {
    this.error = 'Hello, I am an ES6 clas that can be used as an error.'; 
  }
};

const functionThatThrows = function () {
  throw new Error('foo');
};

console.log('The next line has an assertion that should throw an AssertionError.');
assert.throws(functionThatThrows, ES6Error); // blows up with TypeError

[Trott]

Suggested fix in #4166

[Trott]

This is fixed in master and should be in 5.2.1 (or 5.3.0 if 5.2.1 is not the next release).