Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

errors: add internal/errors.js #11220

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 141 additions & 0 deletions doc/guides/using-internal-errors.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
# Using the internal/errors.js Module

## What is internal/errors.js

The `require('internal/errors')` module is an internal-only module that can be
used to produce `Error`, `TypeError` and `RangeError` instances that use a
static, permanent error code and an optionally parameterized message.

The intent of the module is to allow errors provided by Node.js to be assigned a
permanent identifier. Without a permanent identifier, userland code may need to
inspect error messages to distinguish one error from another. An unfortunate
result of that practice is that changes to error messages result in broken code
in the ecosystem. For that reason, Node.js has considered error message changes
to be breaking changes. By providing a permanent identifier for a specific
error, we reduce the need for userland code to inspect error messages.

*Note*: Switching an existing error to use the `internal/errors` module must be
considered a `semver-major` change. However, once using `internal/errors`,
changes to `internal/errors` error messages will be handled as `semver-minor`
or `semver-patch`.

## Using internal/errors.js

The `internal/errors` module exposes three custom `Error` classes that
are intended to replace existing `Error` objects within the Node.js source.

For instance, an existing `Error` such as:

```js
var err = new TypeError('Expected string received ' + type);
```

Can be replaced by first adding a new error key into the `internal/errors.js`
file:

```js
E('FOO', 'Expected string received %s');
```

Then replacing the existing `new TypeError` in the code:

```js
const errors = require('internal/errors');
// ...
var err = new errors.TypeError('FOO', type);
```

## Adding new errors

New static error codes are added by modifying the `internal/errors.js` file
and appending the new error codes to the end using the utility `E()` method.

```js
E('EXAMPLE_KEY1', 'This is the error value');
E('EXAMPLE_KEY2', (a, b) => return `${a} ${b}`);
```

The first argument passed to `E()` is the static identifier. The second
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the name of this function being so short is to echo the existing convention for error names like ENOTFOUND? But this convention is no longer followed in this new error code system?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is short to make it as brief as possible. It is intended to be used only within this file. It is exported only to facilitate testing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is a guide so I think this would be read by contributors, especially those who are new? And they might need to change that file and call E to register new errors.

argument is either a String with optional `util.format()` style replacement
tags (e.g. `%s`, `%d`), or a function returning a String. The optional
additional arguments passed to the `errors.message()` function (which is
used by the `errors.Error`, `errors.TypeError` and `errors.RangeError` classes),
will be used to format the error message.

## Documenting new errors

Whenever a new static error code is added and used, corresponding documentation
for the error code should be added to the `doc/api/errors.md` file. This will
give users a place to go to easily look up the meaning of individual error
codes.


## API

### Class: errors.Error(key[, args...])

* `key` {String} The static error identifier
* `args...` {Any} Zero or more optional arguments

```js
const errors = require('internal/errors');

var arg1 = 'foo';
var arg2 = 'bar';
const myError = new errors.Error('KEY', arg1, arg2);
throw myError;
```

The specific error message for the `myError` instance will depend on the
associated value of `KEY` (see "Adding new errors").

The `myError` object will have a `code` property equal to the `key` and a
`name` property equal to `Error[${key}]`.

### Class: errors.TypeError(key[, args...])

* `key` {String} The static error identifier
* `args...` {Any} Zero or more optional arguments

```js
const errors = require('internal/errors');

var arg1 = 'foo';
var arg2 = 'bar';
const myError = new errors.TypeError('KEY', arg1, arg2);
throw myError;
```

The specific error message for the `myError` instance will depend on the
associated value of `KEY` (see "Adding new errors").

The `myError` object will have a `code` property equal to the `key` and a
`name` property equal to `TypeError[${key}]`.

### Class: errors.RangeError(key[, args...])

* `key` {String} The static error identifier
* `args...` {Any} Zero or more optional arguments

```js
const errors = require('internal/errors');

var arg1 = 'foo';
var arg2 = 'bar';
const myError = new errors.RangeError('KEY', arg1, arg2);
throw myError;
```

The specific error message for the `myError` instance will depend on the
associated value of `KEY` (see "Adding new errors").

The `myError` object will have a `code` property equal to the `key` and a
`name` property equal to `RangeError[${key}]`.

### Method: errors.message(key, args)

* `key` {String} The static error identifier
* `args` {Array} Zero or more optional arguments passed as an Array
* Returns: {String}

Returns the formatted error message string for the given `key`.
88 changes: 88 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
'use strict';

// The whole point behind this internal module is to allow Node.js to no
// longer be forced to treat every error message change as a semver-major
// change. The NodeError classes here all expose a `code` property whose
// value statically and permanently identifies the error. While the error
// message may change, the code should not.

const kCode = Symbol('code');
const messages = new Map();

var assert, util;
function lazyAssert() {
if (!assert)
assert = require('assert');
return assert;
}

function lazyUtil() {
if (!util)
util = require('util');
return util;
}

function makeNodeError(Base) {
return class NodeError extends Base {
constructor(key, ...args) {
super(message(key, args));
this[kCode] = key;
Error.captureStackTrace(this, NodeError);
}

get name() {
return `${super.name}[${this[kCode]}]`;
}

get code() {
return this[kCode];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so if a PR updates a module to throw the new errors, since code is read only here, any code setting .code later would not have effect, then this could break existing behavior unless we check carefully enough before landing those PRs? Can we add a setter that do some kinds of assertion here?

Copy link
Member

@joyeecheung joyeecheung Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

set code(options) {
  if (typeof options === 'object' && options.override === true) {
    this[kCode] = options.code;
    return;
  }
  assert.fail('This error can not be migrated for now');
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: forgot return ^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just leave it read only. If someone really wanted to override the value of .code, they can easily use Object.defineProperty() to do so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then just raise the assertion then? This is just to prevent we accidently migrate an error that already have its .code set by existing code (but it could be hard to catch if that code happens much later than its creation, like in a callback), and by throwing this new kind of error, that code can't actually set .code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no cases that I'm aware of where we set the code later. And in the case a user attempts to set the code, I'd rather they not see an assertion error. We can best catch these cases in code review

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that can not be let loose in the user land. Anyway thanks for bearing with me :P

}
};
}

function message(key, args) {
const assert = lazyAssert();
assert.strictEqual(typeof key, 'string');
const util = lazyUtil();
const msg = messages.get(key);
assert(msg, `An invalid error message key was used: ${key}.`);
let fmt = util.format;
if (typeof msg === 'function') {
fmt = msg;
} else {
if (args === undefined || args.length === 0)
return msg;
args.unshift(msg);
}
return String(fmt.apply(null, args));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmt(...args);? Not sure whether that was part of your conversation with @joyeecheung or not

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That conversation is about rest operator and this is spread operator...not sure about the performance of spread though? From the results of benchmark/misc/console.js with method="restAndSpread" compared method="restAndApply" I think the spread operator is not ready yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like we have a benchmark for the spread operator so I'll open a PR to add one

}

// Utility function for registering the error codes. Only used here. Exported
// *only* to allow for testing.
function E(sym, val) {
messages.set(sym, typeof val === 'function' ? val : String(val));
}

module.exports = exports = {
message,
Error: makeNodeError(Error),
TypeError: makeNodeError(TypeError),
RangeError: makeNodeError(RangeError),
E // This is exported only to facilitate testing.
};

// To declare an error message, use the E(sym, val) function above. The sym
// must be an upper case string. The val can be either a function or a string.
// The return value of the function must be a string.
// Examples:
// E('EXAMPLE_KEY1', 'This is the error value');
// E('EXAMPLE_KEY2', (a, b) => return `${a} ${b}`);
//
// Once an error code has been assigned, the code itself MUST NOT change and
// any given error code must never be reused to identify a different error.
//
// Any error code added here should also be added to the documentation
//
// Note: Please try to keep these in alphabetical order
E('ERR_ASSERTION', (msg) => msg);
// Add new errors from here...
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
'lib/internal/cluster/shared_handle.js',
'lib/internal/cluster/utils.js',
'lib/internal/cluster/worker.js',
'lib/internal/errors.js',
'lib/internal/freelist.js',
'lib/internal/fs.js',
'lib/internal/linkedlist.js',
Expand Down
15 changes: 15 additions & 0 deletions test/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -616,3 +616,18 @@ exports.WPT = {
assert.fail(undefined, undefined, `Reached unreachable code: ${desc}`);
}
};

// Useful for testing expected internal/error objects
exports.expectsError = function expectsError(code, type, message) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function was added to common.js but not documented in the test/README.md.

return function(error) {
assert.strictEqual(error.code, code);
if (type !== undefined)
assert(error instanceof type, 'error is not the expected type');
if (message !== undefined) {
if (!util.isRegExp(message))
message = new RegExp(String(message));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer that if it's a string, match for equality. This way, we don't get weird results due to regexp special characters in strings:

const re = new RegExp(String('foo + bar'));
re.test('foo + bar'); // false!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR? :-D

assert(message.test(error.message), 'error.message does not match');
}
return true;
};
};
116 changes: 116 additions & 0 deletions test/parallel/test-internal-errors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
// Flags: --expose-internals
'use strict';

const common = require('../common');
const errors = require('internal/errors');
const assert = require('assert');

errors.E('TEST_ERROR_1', 'Error for testing purposes: %s');
errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`);

const err1 = new errors.Error('TEST_ERROR_1', 'test');
const err2 = new errors.TypeError('TEST_ERROR_1', 'test');
const err3 = new errors.RangeError('TEST_ERROR_1', 'test');
const err4 = new errors.Error('TEST_ERROR_2', 'abc', 'xyz');

assert(err1 instanceof Error);
assert.strictEqual(err1.name, 'Error[TEST_ERROR_1]');
assert.strictEqual(err1.message, 'Error for testing purposes: test');
assert.strictEqual(err1.code, 'TEST_ERROR_1');

assert(err2 instanceof TypeError);
assert.strictEqual(err2.name, 'TypeError[TEST_ERROR_1]');
assert.strictEqual(err2.message, 'Error for testing purposes: test');
assert.strictEqual(err2.code, 'TEST_ERROR_1');

assert(err3 instanceof RangeError);
assert.strictEqual(err3.name, 'RangeError[TEST_ERROR_1]');
assert.strictEqual(err3.message, 'Error for testing purposes: test');
assert.strictEqual(err3.code, 'TEST_ERROR_1');

assert(err4 instanceof Error);
assert.strictEqual(err4.name, 'Error[TEST_ERROR_2]');
assert.strictEqual(err4.message, 'abc xyz');
assert.strictEqual(err4.code, 'TEST_ERROR_2');

assert.throws(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this block is executed twice?(TEST_FOO_KEY gets passed twice)

() => new errors.Error('TEST_FOO_KEY'),
/^AssertionError: An invalid error message key was used: TEST_FOO_KEY.$/);
// Calling it twice yields same result (using the key does not create it)
assert.throws(
() => new errors.Error('TEST_FOO_KEY'),
/^AssertionError: An invalid error message key was used: TEST_FOO_KEY.$/);
assert.throws(
() => new errors.Error(1),
/^AssertionError: 'number' === 'string'$/);
assert.throws(
() => new errors.Error({}),
/^AssertionError: 'object' === 'string'$/);
assert.throws(
() => new errors.Error([]),
/^AssertionError: 'object' === 'string'$/);
assert.throws(
() => new errors.Error(true),
/^AssertionError: 'boolean' === 'string'$/);
assert.throws(
() => new errors.TypeError(1),
/^AssertionError: 'number' === 'string'$/);
assert.throws(
() => new errors.TypeError({}),
/^AssertionError: 'object' === 'string'$/);
assert.throws(
() => new errors.TypeError([]),
/^AssertionError: 'object' === 'string'$/);
assert.throws(
() => new errors.TypeError(true),
/^AssertionError: 'boolean' === 'string'$/);
assert.throws(
() => new errors.RangeError(1),
/^AssertionError: 'number' === 'string'$/);
assert.throws(
() => new errors.RangeError({}),
/^AssertionError: 'object' === 'string'$/);
assert.throws(
() => new errors.RangeError([]),
/^AssertionError: 'object' === 'string'$/);
assert.throws(
() => new errors.RangeError(true),
/^AssertionError: 'boolean' === 'string'$/);


// Tests for common.expectsError
assert.doesNotThrow(() => {
assert.throws(() => {
throw new errors.TypeError('TEST_ERROR_1', 'a');
}, common.expectsError('TEST_ERROR_1'));
});

assert.doesNotThrow(() => {
assert.throws(() => {
throw new errors.TypeError('TEST_ERROR_1', 'a');
}, common.expectsError('TEST_ERROR_1', TypeError, /^Error for testing/));
});

assert.doesNotThrow(() => {
assert.throws(() => {
throw new errors.TypeError('TEST_ERROR_1', 'a');
}, common.expectsError('TEST_ERROR_1', TypeError));
});

assert.doesNotThrow(() => {
assert.throws(() => {
throw new errors.TypeError('TEST_ERROR_1', 'a');
}, common.expectsError('TEST_ERROR_1', Error));
});

assert.throws(() => {
assert.throws(() => {
throw new errors.TypeError('TEST_ERROR_1', 'a');
}, common.expectsError('TEST_ERROR_1', RangeError));
}, /^AssertionError: error is not the expected type/);

assert.throws(() => {
assert.throws(() => {
throw new errors.TypeError('TEST_ERROR_1', 'a');
}, common.expectsError('TEST_ERROR_1', TypeError, /^Error for testing 2/));
}, /^AssertionError: error.message does not match/);