From 92c74967ff380ce0e373322ffa12ead4f3e08130 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Tue, 6 Mar 2018 14:17:37 +0100 Subject: [PATCH 1/4] lib: remove unused internal error constructors --- lib/internal/errors.js | 14 +-- test/parallel/test-internal-errors.js | 133 +++++--------------------- test/parallel/test-util.js | 2 +- 3 files changed, 28 insertions(+), 121 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index fd93547d26d37e..8af2ff164e2be7 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -125,7 +125,6 @@ function makeNodeErrorWithCode(Base, key) { // *only* to allow for testing. function E(sym, val, def, ...otherClasses) { messages.set(sym, val); - if (def === undefined) return; def = makeNodeErrorWithCode(def, sym); if (otherClasses.length !== 0) { otherClasses.forEach((clazz) => { @@ -572,10 +571,6 @@ module.exports = exports = { exceptionWithHostPort, uvException, message, - Error: makeNodeError(Error), - TypeError: makeNodeError(TypeError), - RangeError: makeNodeError(RangeError), - URIError: makeNodeError(URIError), AssertionError, SystemError, codes, @@ -583,12 +578,13 @@ module.exports = exports = { errorCache: new Map() // This is in here only to facilitate testing. }; -// To declare an error message, use the E(sym, val) function above. The sym +// To declare an error message, use the E(sym, val, def) function above. The sym // must be an upper case string. The val can be either a function or a string. +// The def must be an error class. // 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}`); +// E('EXAMPLE_KEY1', 'This is the error value', Error); +// E('EXAMPLE_KEY2', (a, b) => return `${a} ${b}`, RangeError); // // 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. @@ -858,7 +854,6 @@ E('ERR_STREAM_UNSHIFT_AFTER_END_EVENT', 'stream.unshift() after end event', Error); E('ERR_STREAM_WRAP', 'Stream has StringDecoder set or is in objectMode', Error); E('ERR_STREAM_WRITE_AFTER_END', 'write after end', Error); -E('ERR_SYSTEM_ERROR', sysError); E('ERR_TLS_CERT_ALTNAME_INVALID', 'Hostname/IP does not match certificate\'s altnames: %s', Error); E('ERR_TLS_DH_PARAM_SIZE', 'DH parameter size %s is less than 2048', Error); @@ -934,6 +929,7 @@ function sysError(code, syscall, path, dest, } return message; } +messages.set('ERR_SYSTEM_ERROR', sysError); function invalidArgType(name, expected, actual) { internalAssert(name, 'name is required'); diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index 9bce8a59eafa5b..dd6491ebce5219 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -5,15 +5,12 @@ const common = require('../common'); const assert = require('assert'); const errors = require('internal/errors'); -function invalidKey(key) { - return new RegExp(`^An invalid error message key was used: ${key}\\.$`); -} - -errors.E('TEST_ERROR_1', 'Error for testing purposes: %s'); -errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`); +errors.E('TEST_ERROR_1', 'Error for testing purposes: %s', + Error, TypeError, RangeError); +errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`, Error); { - const err = new errors.Error('TEST_ERROR_1', 'test'); + const err = new errors.codes.TEST_ERROR_1('test'); assert(err instanceof Error); assert.strictEqual(err.name, 'Error [TEST_ERROR_1]'); assert.strictEqual(err.message, 'Error for testing purposes: test'); @@ -21,7 +18,7 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`); } { - const err = new errors.TypeError('TEST_ERROR_1', 'test'); + const err = new errors.codes.TEST_ERROR_1.TypeError('test'); assert(err instanceof TypeError); assert.strictEqual(err.name, 'TypeError [TEST_ERROR_1]'); assert.strictEqual(err.message, 'Error for testing purposes: test'); @@ -29,7 +26,7 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`); } { - const err = new errors.RangeError('TEST_ERROR_1', 'test'); + const err = new errors.codes.TEST_ERROR_1.RangeError('test'); assert(err instanceof RangeError); assert.strictEqual(err.name, 'RangeError [TEST_ERROR_1]'); assert.strictEqual(err.message, 'Error for testing purposes: test'); @@ -37,7 +34,7 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`); } { - const err = new errors.Error('TEST_ERROR_2', 'abc', 'xyz'); + const err = new errors.codes.TEST_ERROR_2('abc', 'xyz'); assert(err instanceof Error); assert.strictEqual(err.name, 'Error [TEST_ERROR_2]'); assert.strictEqual(err.message, 'abc xyz'); @@ -45,113 +42,27 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`); } { - const err = new errors.Error('TEST_ERROR_1'); + const err = new errors.codes.TEST_ERROR_1(); assert(err instanceof Error); assert.strictEqual(err.name, 'Error [TEST_ERROR_1]'); assert.strictEqual(err.message, 'Error for testing purposes: %s'); assert.strictEqual(err.code, 'TEST_ERROR_1'); } -common.expectsError( - () => new errors.Error('TEST_FOO_KEY'), - { - code: 'ERR_ASSERTION', - message: invalidKey('TEST_FOO_KEY') - }); -// Calling it twice yields same result (using the key does not create it) -common.expectsError( - () => new errors.Error('TEST_FOO_KEY'), - { - code: 'ERR_ASSERTION', - message: invalidKey('TEST_FOO_KEY') - }); -common.expectsError( - () => new errors.Error(1), - { - code: 'ERR_ASSERTION', - message: invalidKey(1) - }); -common.expectsError( - () => new errors.Error({}), - { - code: 'ERR_ASSERTION', - message: invalidKey('\\[object Object\\]') - }); -common.expectsError( - () => new errors.Error([]), - { - code: 'ERR_ASSERTION', - message: invalidKey('') - }); -common.expectsError( - () => new errors.Error(true), - { - code: 'ERR_ASSERTION', - message: invalidKey('true') - }); -common.expectsError( - () => new errors.TypeError(1), - { - code: 'ERR_ASSERTION', - message: invalidKey(1) - }); -common.expectsError( - () => new errors.TypeError({}), - { - code: 'ERR_ASSERTION', - message: invalidKey('\\[object Object\\]') - }); -common.expectsError( - () => new errors.TypeError([]), - { - code: 'ERR_ASSERTION', - message: invalidKey('') - }); -common.expectsError( - () => new errors.TypeError(true), - { - code: 'ERR_ASSERTION', - message: invalidKey('true') - }); -common.expectsError( - () => new errors.RangeError(1), - { - code: 'ERR_ASSERTION', - message: invalidKey(1) - }); -common.expectsError( - () => new errors.RangeError({}), - { - code: 'ERR_ASSERTION', - message: invalidKey('\\[object Object\\]') - }); -common.expectsError( - () => new errors.RangeError([]), - { - code: 'ERR_ASSERTION', - message: invalidKey('') - }); -common.expectsError( - () => new errors.RangeError(true), - { - code: 'ERR_ASSERTION', - message: invalidKey('true') - }); - // Tests for common.expectsError common.expectsError(() => { - throw new errors.TypeError('TEST_ERROR_1', 'a'); + throw new errors.codes.TEST_ERROR_1.TypeError('a'); }, { code: 'TEST_ERROR_1' }); common.expectsError(() => { - throw new errors.TypeError('TEST_ERROR_1', 'a'); + throw new errors.codes.TEST_ERROR_1.TypeError('a'); }, { code: 'TEST_ERROR_1', type: TypeError, message: /^Error for testing/ }); common.expectsError(() => { - throw new errors.TypeError('TEST_ERROR_1', 'a'); + throw new errors.codes.TEST_ERROR_1.TypeError('a'); }, { code: 'TEST_ERROR_1', type: TypeError }); common.expectsError(() => { - throw new errors.TypeError('TEST_ERROR_1', 'a'); + throw new errors.codes.TEST_ERROR_1.TypeError('a'); }, { code: 'TEST_ERROR_1', type: TypeError, @@ -160,7 +71,7 @@ common.expectsError(() => { common.expectsError(() => { common.expectsError(() => { - throw new errors.TypeError('TEST_ERROR_1', 'a'); + throw new errors.codes.TEST_ERROR_1.TypeError('a'); }, { code: 'TEST_ERROR_1', type: RangeError }); }, { code: 'ERR_ASSERTION', @@ -169,7 +80,7 @@ common.expectsError(() => { common.expectsError(() => { common.expectsError(() => { - throw new errors.TypeError('TEST_ERROR_1', 'a'); + throw new errors.codes.TEST_ERROR_1.TypeError('a'); }, { code: 'TEST_ERROR_1', type: TypeError, message: /^Error for testing 2/ }); @@ -318,7 +229,7 @@ assert.strictEqual( { const { kMaxLength } = process.binding('buffer'); - const error = new errors.Error('ERR_BUFFER_TOO_LARGE'); + const error = new errors.codes.ERR_BUFFER_TOO_LARGE(); assert.strictEqual( error.message, `Cannot create a Buffer larger than 0x${kMaxLength.toString(16)} bytes` @@ -326,7 +237,7 @@ assert.strictEqual( } { - const error = new errors.Error('ERR_INVALID_ARG_VALUE', 'foo', '\u0000bar'); + const error = new errors.codes.ERR_INVALID_ARG_VALUE('foo', '\u0000bar'); assert.strictEqual( error.message, 'The argument \'foo\' is invalid. Received \'\\u0000bar\'' @@ -334,9 +245,9 @@ assert.strictEqual( } { - const error = new errors.Error( - 'ERR_INVALID_ARG_VALUE', - 'foo', { a: 1 }, 'must have property \'b\''); + const error = new errors.codes.ERR_INVALID_ARG_VALUE( + 'foo', { a: 1 }, 'must have property \'b\'' + ); assert.strictEqual( error.message, 'The argument \'foo\' must have property \'b\'. Received { a: 1 }' @@ -346,7 +257,7 @@ assert.strictEqual( // Test that `code` property is mutable and that changing it does not change the // name. { - const myError = new errors.Error('ERR_TLS_HANDSHAKE_TIMEOUT'); + const myError = new errors.codes.ERR_TLS_HANDSHAKE_TIMEOUT(); assert.strictEqual(myError.code, 'ERR_TLS_HANDSHAKE_TIMEOUT'); assert.strictEqual(myError.hasOwnProperty('code'), false); assert.strictEqual(myError.hasOwnProperty('name'), false); @@ -364,7 +275,7 @@ assert.strictEqual( // `console.log()` results, which is the behavior of `Error` objects in the // browser. Note that `name` becomes enumerable after being assigned. { - const myError = new errors.Error('ERR_TLS_HANDSHAKE_TIMEOUT'); + const myError = new errors.codes.ERR_TLS_HANDSHAKE_TIMEOUT(); assert.deepStrictEqual(Object.keys(myError), []); const initialToString = myError.toString(); @@ -379,7 +290,7 @@ assert.strictEqual( { let initialConsoleLog = ''; common.hijackStdout((data) => { initialConsoleLog += data; }); - const myError = new errors.Error('ERR_TLS_HANDSHAKE_TIMEOUT'); + const myError = new errors.codes.ERR_TLS_HANDSHAKE_TIMEOUT(); assert.deepStrictEqual(Object.keys(myError), []); const initialToString = myError.toString(); console.log(myError); diff --git a/test/parallel/test-util.js b/test/parallel/test-util.js index 72d4b16b353102..10ad1db4eeb2ba 100644 --- a/test/parallel/test-util.js +++ b/test/parallel/test-util.js @@ -170,7 +170,7 @@ util.error('test'); assert.strictEqual(util.types.isNativeError(Object.create(Error.prototype)), false); assert.strictEqual( - util.types.isNativeError(new errors.Error('ERR_IPC_CHANNEL_CLOSED')), + util.types.isNativeError(new errors.codes.ERR_IPC_CHANNEL_CLOSED()), true ); } From 653c7ed5ad8b45397bb718f8bb60ccbc83870175 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Tue, 6 Mar 2018 14:23:36 +0100 Subject: [PATCH 2/4] doc: update internal errors documentation --- CPP_STYLE_GUIDE.md | 4 +- doc/guides/using-internal-errors.md | 92 ++++++++--------------------- 2 files changed, 28 insertions(+), 68 deletions(-) diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md index 5a275094ad16fa..edc5f0f12e212c 100644 --- a/CPP_STYLE_GUIDE.md +++ b/CPP_STYLE_GUIDE.md @@ -256,13 +256,13 @@ env->SetMethod(target, "foo", Foo); exports.foo = function(str) { // Prefer doing the type-checks in JavaScript if (typeof str !== 'string') { - throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'str', 'string'); + throw new errors.codes.ERR_INVALID_ARG_TYPE('str', 'string'); } const ctx = {}; const result = binding.foo(str, ctx); if (ctx.code !== undefined) { - throw new errors.Error('ERR_ERROR_NAME', ctx.code); + throw new errors.codes.ERR_ERROR_NAME(ctx.code); } return result; }; diff --git a/doc/guides/using-internal-errors.md b/doc/guides/using-internal-errors.md index c03f44623a0f7f..57e0fe8e36140d 100644 --- a/doc/guides/using-internal-errors.md +++ b/doc/guides/using-internal-errors.md @@ -19,8 +19,8 @@ considered a `semver-major` change. ## 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. +The `internal/errors` module exposes all custom errors as subclasses of the +builtin errors. After being added, an error can be found in the `codes` object. For instance, an existing `Error` such as: @@ -32,15 +32,15 @@ Can be replaced by first adding a new error key into the `internal/errors.js` file: ```js -E('FOO', 'Expected string received %s'); +E('FOO', 'Expected string received %s', TypeError); ``` Then replacing the existing `new TypeError` in the code: ```js -const errors = require('internal/errors'); +const { FOO } = require('internal/errors').codes; // ... -const err = new errors.TypeError('FOO', type); +const err = new FOO(type); ``` ## Adding new errors @@ -49,8 +49,8 @@ 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) => `${a} ${b}`); +E('EXAMPLE_KEY1', 'This is the error value', TypeError); +E('EXAMPLE_KEY2', (a, b) => `${a} ${b}`, RangeError); ``` The first argument passed to `E()` is the static identifier. The second @@ -58,7 +58,23 @@ 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. +will be used to format the error message. The third argument is the base class +that the new error will extend. + +It is possible to create multiple derived +classes by providing additional arguments. The other ones will be exposed as +properties of the main class: + +```js +E('EXAMPLE_KEY', 'Error message', TypeError, RangeError); + +// In another module +const { EXAMPLE_KEY } = require('internal/errors').codes; +// TypeError +throw new EXAMPLE_KEY(); +// RangeError +throw new EXAMPLE_KEY.RangeError(); +``` ## Documenting new errors @@ -115,65 +131,9 @@ likely be required. ## 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'); - -const arg1 = 'foo'; -const 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'); - -const arg1 = 'foo'; -const 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'); - -const arg1 = 'foo'; -const 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"). +### Object: errors.codes -The `myError` object will have a `code` property equal to the `key` and a -`name` property equal to `` `RangeError [${key}]` ``. +Exposes all internal error classes to be used by Node.js APIs. ### Method: errors.message(key, args) From f11a5cd5f14c13465dd738e3a9a226f4a68e8671 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 8 Mar 2018 09:12:35 +0100 Subject: [PATCH 3/4] fix doc linter --- doc/guides/using-internal-errors.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/guides/using-internal-errors.md b/doc/guides/using-internal-errors.md index 57e0fe8e36140d..d22fc68e0f34c4 100644 --- a/doc/guides/using-internal-errors.md +++ b/doc/guides/using-internal-errors.md @@ -65,6 +65,7 @@ It is possible to create multiple derived classes by providing additional arguments. The other ones will be exposed as properties of the main class: + ```js E('EXAMPLE_KEY', 'Error message', TypeError, RangeError); From c6c904e112365e6b0ff67e65f032f00779878a4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 8 Mar 2018 09:12:45 +0100 Subject: [PATCH 4/4] migrate new fs errors --- lib/fs.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 321fbe8d54ef8e..f890e431d2a9f5 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -36,6 +36,8 @@ const fs = exports; const { Buffer } = require('buffer'); const errors = require('internal/errors'); const { + ERR_FS_WATCHER_ALREADY_STARTED, + ERR_FS_WATCHER_NOT_STARTED, ERR_INVALID_ARG_TYPE, ERR_INVALID_CALLBACK, ERR_OUT_OF_RANGE @@ -1349,7 +1351,7 @@ FSWatcher.prototype.start = function(filename, encoding) { lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent'); if (this._handle.initialized) { - throw new errors.Error('ERR_FS_WATCHER_ALREADY_STARTED'); + throw new ERR_FS_WATCHER_ALREADY_STARTED(); } filename = getPathFromURL(filename); @@ -1373,7 +1375,7 @@ FSWatcher.prototype.start = function(filename, FSWatcher.prototype.close = function() { lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent'); if (!this._handle.initialized) { - throw new errors.Error('ERR_FS_WATCHER_NOT_STARTED'); + throw new ERR_FS_WATCHER_NOT_STARTED(); } this._handle.close(); };