errors thrown from a vm aren't recognized as errors

[boneskull]

As you can read in the comments on commit 02d421b, it seems that if code under test uses vm.runInNewContext() and throws an Error:

try {
  vm.runInNewContext('throw new TypeError()');
} catch (e) {
  e instanceof Error; // false
}

I think we can safeguard against this by duck-typing instead of an instanceof check.

[boneskull]

cc @sarkiroka, @jakecraige, @danielstjules

[danielstjules]

I think this is fixed in master, from #1758: https://github.com/mochajs/mocha/blob/master/lib/runner.js#L205-L207

try {
  vm.runInNewContext('throw new TypeError()');
} catch (err) {
  console.log(err instanceof Error || err && typeof err.message == 'string');
}
// => true

[boneskull]

@danielstjules OK. That "works", but checking the log, the intent was to solve cross-frame issues, not open it up to throwing plain-jane objects.

I like lodash.isError() for this:

var errorTag = '[object Error]';
var objectProto = Object.prototype;
var objToString = objectProto.toString;

function isError(value) {
  return isObjectLike(value) && typeof value.message == 'string' && objToString.call(value) == errorTag;
}

No instanceof to be found. Don't think it's necessary.

[danielstjules]

But that wouldn't work with custom error objects cross-frame (or in the same context, depending on how it's used)

var errorTag = '[object Error]';
var objectProto = Object.prototype;
var objToString = objectProto.toString;

function isError(value) {
  return typeof value.message == 'string' && objToString.call(value) == errorTag;
}

function FooBarError(message) { this.message = message; }
FooBarError.prototype = new TypeError();

isError(new FooBarError('test'));
// => false

[boneskull]

@danielstjules No, it wouldn't; not like that, anyway. That isn't an Error instance either way you look at it...

[boneskull]

there are comments in #1770 but let's keep the discussion here.

[danielstjules]

How is it not an error instance any way you look at it? It's an error-like object?

var utils = require('util');
var errorTag = '[object Error]';
var objectProto = Object.prototype;
var objToString = objectProto.toString;

function isError(value) {
  return typeof value.message == 'string' && objToString.call(value) == errorTag;
}

function FooBarError(message) {
  this.message = message;
}

util.inherits(FooBarError, TypeError);

isError(new FooBarError('test'));
// => false

var utils = require('util');
var errorTag = '[object Error]';
var objectProto = Object.prototype;
var objToString = objectProto.toString;

function isError(value) {
  return typeof value.message == 'string' && objToString.call(value) == errorTag;
}

function FooBarError(message) {
  this.message = message;
}

util.inherits(FooBarError, Error);

isError(new FooBarError('test'));
// => false

[danielstjules]

I think we're stuck just checking for the presence of err.message

[boneskull]

@danielstjules I think maybe I haven't made my intent clear. Above, I wrote:

the intent was to solve cross-frame issues, not open it up to throwing plain-jane objects.

The code you wrote does not result in an Error instance--it's an Error-like object--but if you throw a TypeError from another frame, isError() will recognize it. That's the intent of the original patch--not to support "custom" classes which happen to have the same prototype as TypeError.

At any rate, there's no good way to "subclass" Error outside of ES6. I've done a fair amount of research on the topic, and can only deduce that if you're not using ES6, you're better off putting custom properties on an Error instance.

If you're using ES6, you can of course extend TypeError if you wish.

[boneskull]

(I'm unsure where the idea that throwing {message: 'foo'} was ok came from)

[boneskull]

So, that's why I targeted the #1771 to v3.0.0--because currently, you can throw a function with a message property if you want--but I'm pretty sure that's not something we should be supporting.

[boneskull]

How is it not an error instance any way you look at it?

because

new FooBarError('test') instanceof Error  // false

thus not an Error instance

[danielstjules]

thus not an Error instance

That's not true. It is an instance of Error, and it's a common practice for defining custom error objects. See below:

var utils = require('util');
var errorTag = '[object Error]';
var objectProto = Object.prototype;
var objToString = objectProto.toString;

function isError(value) {
  return typeof value.message == 'string' && objToString.call(value) == errorTag;
}

function FooBarError(message) {
  this.message = message;
}

util.inherits(FooBarError, Error);
(new FooBarError('test') instanceof Error);
// true

Tested with iojs v1.6.2 and node 0.12.

[boneskull]

@danielstjules my bad. I got like 3 hours of sleep, sorry.

[boneskull]

it's a common practice for defining custom error objects

I would vehemently argue it's a poor one

[danielstjules]

Even node src has example of this:
https://github.com/nodejs/io.js/blob/b5cd2f098691935b6bef6ded1b0de7ef37431f27/test/parallel/test-util-format.js#L59-L67
https://github.com/nodejs/io.js/blob/06721fe00589d9ad61fe8defcd31c5f40df72c9b/lib/repl.js#L1005
https://github.com/nodejs/io.js/blob/2a7fd0ad328d5197a9a166650e9eaa51367e3152/lib/assert.js#L61

I'm not sure how we can ignore custom errors defined in this way.

[danielstjules]

No worries haha I know the feeling.

[danielstjules]

I would vehemently argue it's a poor one

I don't think inheriting the prototype from Error is odd at all. That's exactly how you should be able to define custom errors in JS. I agree that defining plain old objects with a message property alone is a terrible approach though. But for a catch-all solution, I think we're stuck with only checking for a message property. Which I understand def sucks lol

[boneskull]

I guess we're getting philosophical.

You can do it, but look at the AssertionError constructor, and all the garbage you need in there to make it work properly. Of course, the same code won't necessarily work in any given browser, or against any given JVM. at the top of the file:

// THIS IS NOT TESTED NOR LIKELY TO WORK OUTSIDE V8!

I don't think inheriting the prototype from Error is odd at all. That's exactly how you should be able to define custom errors in JS. I agree that defining plain old objects with a message property alone is a terrible solution. But for a catch-all solution, I think we're stuck with only checking for a message property.

It's not "odd"--it's commonplace, as you said--it's just completely unnecessary. Find the usages of AssertionError in the io.js source. now, pretend the thing which threw the AssertionError threw this instead:

var assertionError = new Error();
assertionError.type = 'AssertionError';
throw assertionError;

Then, look at those usages again and tell me where they couldn't simply compare type instead of name. My point is that there's absolutely no reason to "subclass" Error, especially considering how weak the catch keyword is.

[boneskull]

(even in ES6, the value is marginal is probably performance-related, because the language doesn't have things like try/catch/catch/catch blocks)

[danielstjules]

Fair enough. :) I agree that "sub-classing" Error doesn't get you much given how weak try/catch is in JS.

As for AssertionError - that constructor seems over-complicated. Just inheriting the proto works in v8 for capturing the stack trace. SpiderMonkey doesn't seem to care for it though.

function FooBarError(message) { this.message = message; }
FooBarError.prototype = new Error();

function test() {
  throw new FooBarError('oh no!');
}

test();

[danielstjules]

As a result of the behavior shown in #1770 (comment) and #1770 (comment), do you agree that we'll have to resort to solely checking for a message property on the object?

[boneskull]

Yeah I guess. It depends how opinionated we want to get. As a testing framework we probably shouldn't be dictating how people write their code. My point stands that subclassing Error in JS is a fairly worthless exercise, but I don't think we need to punish users if they choose to do it.

[boneskull]

On the flip side though, I'm 99.9999% sure throwing a string has no use case and shouldn't be supported. 😄

[danielstjules]

Agreed!