errors: add internal/errors.js by jasnell · Pull Request #11220 · nodejs/node · GitHub
Skip to content
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
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
Copy Markdown
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?

@joyeecheung joyeecheung Feb 14, 2017

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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