From eeab1b554c17ad795351ca0789276f6a66a44e1a Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Sat, 18 Sep 2021 15:38:55 +0200 Subject: [PATCH 1/8] Add tests for error properties --- test.js | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/test.js b/test.js index 8f3e995..c60e766 100644 --- a/test.js +++ b/test.js @@ -14,9 +14,63 @@ test('all errors are instances of LevelUPError', function (t) { t.end() }) +test('error with message has expected properties', function (t) { + const error = new errors.ReadError('foo') + + t.is(error.type, 'ReadError') + t.is(error.name, 'ReadError') + t.is(error.cause, undefined) + t.is(error.message, 'foo') + t.end() +}) + +test('error without message has expected properties', function (t) { + const error = new errors.ReadError() + + t.is(error.type, 'ReadError') + t.is(error.name, 'ReadError') + t.is(error.cause, undefined) + // t.is(error.message, '') // TODO: bug + t.end() +}) + +test('error with cause has expected properties', function (t) { + const cause = new Error('foo') + const error = new errors.ReadError(cause) + + t.is(error.type, 'ReadError') + t.is(error.name, 'ReadError') + // t.ok(error.cause === cause) // TODO: bug in errno + t.is(error.message, 'foo') + t.end() +}) + +test('error with message and cause has expected properties', function (t) { + const cause = new Error('foo') + const error = new errors.ReadError('bar', cause) + + t.is(error.type, 'ReadError') + t.is(error.name, 'ReadError') + t.ok(error.cause === cause) + t.is(error.message, 'bar') + t.end() +}) + test('NotFoundError has special properties', function (t) { const error = new errors.NotFoundError() - t.equal(error.notFound, true) - t.equal(error.status, 404) + t.is(error.notFound, true) + t.is(error.status, 404) + t.end() +}) + +test.skip('error message is writable', function (t) { + // TODO: bug in prr? Given shorthand 'ewr', the effective options are: + // { enumerable: false, configurable: false, writable: false } + // Let's keep enumerable as-is, but set configurable and writable for flexibility. + // That's also the same as the `message` property of a new Error('foo'). + + const error = new errors.WriteError('foo') + error.message = 'Got error: ' + error.message + t.is(error.message, 'Got error: foo') t.end() }) From 3576ead7edf7e2597269b26c18a03f4914be1a5a Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Sat, 18 Sep 2021 15:41:34 +0200 Subject: [PATCH 2/8] Inline `errno` package --- errors.js | 38 ++++++++++++++++++++++++++++++++------ package.json | 4 +--- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/errors.js b/errors.js index 01b073c..3279d89 100644 --- a/errors.js +++ b/errors.js @@ -1,11 +1,34 @@ 'use strict' -const createError = require('errno').create -const LevelUPError = createError('LevelUPError') -const NotFoundError = createError('NotFoundError', LevelUPError) +function createError (type, Proto) { + const Err = function (message, cause) { + if (message && typeof message !== 'string') { + message = message.message || message.name + } + + // can be passed just a 'cause' + cause = typeof message !== 'string' ? message : cause + + Object.defineProperty(this, 'type', { value: type, enumerable: false, writable: false, configurable: false }) + Object.defineProperty(this, 'name', { value: type, enumerable: false, writable: false, configurable: false }) + Object.defineProperty(this, 'cause', { value: cause, enumerable: false, writable: false, configurable: false }) + Object.defineProperty(this, 'message', { value: message, enumerable: false, writable: false, configurable: false }) + + Error.call(this) + + if (typeof Error.captureStackTrace === 'function') { + Error.captureStackTrace(this, Err) + } + } -NotFoundError.prototype.notFound = true -NotFoundError.prototype.status = 404 + if (Proto != null) { + Err.prototype = new Proto() + } + + return Err +} + +const LevelUPError = createError('LevelUPError') module.exports = { LevelUPError: LevelUPError, @@ -13,6 +36,9 @@ module.exports = { OpenError: createError('OpenError', LevelUPError), ReadError: createError('ReadError', LevelUPError), WriteError: createError('WriteError', LevelUPError), - NotFoundError: NotFoundError, + NotFoundError: createError('NotFoundError', LevelUPError), EncodingError: createError('EncodingError', LevelUPError) } + +module.exports.NotFoundError.prototype.notFound = true +module.exports.NotFoundError.prototype.status = 404 diff --git a/package.json b/package.json index ee2308a..ea913a4 100644 --- a/package.json +++ b/package.json @@ -19,9 +19,7 @@ "LICENSE.md", "UPGRADING.md" ], - "dependencies": { - "errno": "^1.0.0" - }, + "dependencies": {}, "devDependencies": { "airtap": "^4.0.3", "airtap-playwright": "^1.0.1", From cb62efa7228ed29a91dd2ec1a46faa74a0bbe0b7 Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Sat, 18 Sep 2021 15:52:20 +0200 Subject: [PATCH 3/8] Fix `message` and `cause` bugs --- errors.js | 7 +++---- test.js | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/errors.js b/errors.js index 3279d89..c829f73 100644 --- a/errors.js +++ b/errors.js @@ -3,16 +3,15 @@ function createError (type, Proto) { const Err = function (message, cause) { if (message && typeof message !== 'string') { + // Can be passed just a cause + cause = cause || message message = message.message || message.name } - // can be passed just a 'cause' - cause = typeof message !== 'string' ? message : cause - Object.defineProperty(this, 'type', { value: type, enumerable: false, writable: false, configurable: false }) Object.defineProperty(this, 'name', { value: type, enumerable: false, writable: false, configurable: false }) Object.defineProperty(this, 'cause', { value: cause, enumerable: false, writable: false, configurable: false }) - Object.defineProperty(this, 'message', { value: message, enumerable: false, writable: false, configurable: false }) + Object.defineProperty(this, 'message', { value: message || '', enumerable: false, writable: false, configurable: false }) Error.call(this) diff --git a/test.js b/test.js index c60e766..84dbdd0 100644 --- a/test.js +++ b/test.js @@ -30,7 +30,7 @@ test('error without message has expected properties', function (t) { t.is(error.type, 'ReadError') t.is(error.name, 'ReadError') t.is(error.cause, undefined) - // t.is(error.message, '') // TODO: bug + t.is(error.message, '') t.end() }) @@ -40,7 +40,7 @@ test('error with cause has expected properties', function (t) { t.is(error.type, 'ReadError') t.is(error.name, 'ReadError') - // t.ok(error.cause === cause) // TODO: bug in errno + t.ok(error.cause === cause) t.is(error.message, 'foo') t.end() }) From 6db33c7d76597f76801f87146a80f20205e06018 Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Sat, 18 Sep 2021 15:54:34 +0200 Subject: [PATCH 4/8] Make error properties configurable and writable --- errors.js | 8 ++++---- test.js | 7 +------ 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/errors.js b/errors.js index c829f73..2648bb1 100644 --- a/errors.js +++ b/errors.js @@ -8,10 +8,10 @@ function createError (type, Proto) { message = message.message || message.name } - Object.defineProperty(this, 'type', { value: type, enumerable: false, writable: false, configurable: false }) - Object.defineProperty(this, 'name', { value: type, enumerable: false, writable: false, configurable: false }) - Object.defineProperty(this, 'cause', { value: cause, enumerable: false, writable: false, configurable: false }) - Object.defineProperty(this, 'message', { value: message || '', enumerable: false, writable: false, configurable: false }) + Object.defineProperty(this, 'type', { value: type, enumerable: false, writable: true, configurable: true }) + Object.defineProperty(this, 'name', { value: type, enumerable: false, writable: true, configurable: true }) + Object.defineProperty(this, 'cause', { value: cause, enumerable: false, writable: true, configurable: true }) + Object.defineProperty(this, 'message', { value: message || '', enumerable: false, writable: true, configurable: true }) Error.call(this) diff --git a/test.js b/test.js index 84dbdd0..c46d807 100644 --- a/test.js +++ b/test.js @@ -63,12 +63,7 @@ test('NotFoundError has special properties', function (t) { t.end() }) -test.skip('error message is writable', function (t) { - // TODO: bug in prr? Given shorthand 'ewr', the effective options are: - // { enumerable: false, configurable: false, writable: false } - // Let's keep enumerable as-is, but set configurable and writable for flexibility. - // That's also the same as the `message` property of a new Error('foo'). - +test('error message is writable for flexibility', function (t) { const error = new errors.WriteError('foo') error.message = 'Got error: ' + error.message t.is(error.message, 'Got error: foo') From 8c2de6b5561f6df9717f09ecddd24f8b8381b0a8 Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Sat, 18 Sep 2021 16:08:04 +0200 Subject: [PATCH 5/8] Don't wrap existing errors So that when `abstract-leveldown` starts using `level-errors`, and such a db is wrapped with `levelup`, `levelup` will not rewrap errors and lose stack trace information in the process. I.e.: ``` // Error created by abstract-leveldown const err = new ReadError('example') // Error created by levelup const wrapped = new ReadError(err) assert(wrapped === err) ``` Ref https://github.com/Level/community/issues/58 --- errors.js | 13 +++++++++++-- test.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/errors.js b/errors.js index 2648bb1..2aeddf9 100644 --- a/errors.js +++ b/errors.js @@ -2,16 +2,25 @@ function createError (type, Proto) { const Err = function (message, cause) { - if (message && typeof message !== 'string') { + if (typeof message === 'object' && message !== null) { // Can be passed just a cause cause = cause || message message = message.message || message.name } + message = message || '' + cause = cause || undefined + + // If input is already of type, return as-is to keep its stack trace. + // Avoid instanceof, for when node_modules has multiple copies of level-errors. + if (typeof cause === 'object' && cause.type === type && cause.message === message) { + return cause + } + Object.defineProperty(this, 'type', { value: type, enumerable: false, writable: true, configurable: true }) Object.defineProperty(this, 'name', { value: type, enumerable: false, writable: true, configurable: true }) Object.defineProperty(this, 'cause', { value: cause, enumerable: false, writable: true, configurable: true }) - Object.defineProperty(this, 'message', { value: message || '', enumerable: false, writable: true, configurable: true }) + Object.defineProperty(this, 'message', { value: message, enumerable: false, writable: true, configurable: true }) Error.call(this) diff --git a/test.js b/test.js index c46d807..da5fa54 100644 --- a/test.js +++ b/test.js @@ -69,3 +69,33 @@ test('error message is writable for flexibility', function (t) { t.is(error.message, 'Got error: foo') t.end() }) + +test('returns original instance if cause is the same type', function (t) { + const cause = new errors.NotFoundError('Key not found in database [foo]') + const error = new errors.NotFoundError(cause) + t.ok(cause === error, 'same instance') + t.is(error.message, 'Key not found in database [foo]') + t.end() +}) + +test('returns new instance if cause prototype is different', function (t) { + const cause = new errors.NotFoundError('Key not found in database [foo]') + const error = new errors.WriteError(cause) + t.ok(cause !== error, 'new instance') + t.is(error.message, 'Key not found in database [foo]') + t.end() +}) + +test('returns original instance if message and cause are the same', function (t) { + const cause = new errors.NotFoundError('Key not found in database [foo]') + const error = new errors.NotFoundError('Key not found in database [foo]', cause) + t.ok(cause === error, 'same instance') + t.end() +}) + +test('returns new instance if message is different', function (t) { + const cause = new errors.NotFoundError('Key not found in database [foo]') + const error = new errors.NotFoundError('Key not found in database [bar]', cause) + t.ok(cause !== error, 'new instance') + t.end() +}) From 4b77e8ff4ef332ecab9f2df50af6bb1da47ba7b9 Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Sun, 19 Sep 2021 14:16:09 +0200 Subject: [PATCH 6/8] Fix Error prototype --- errors.js | 7 ++----- test.js | 3 ++- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/errors.js b/errors.js index 2aeddf9..036eba7 100644 --- a/errors.js +++ b/errors.js @@ -29,14 +29,11 @@ function createError (type, Proto) { } } - if (Proto != null) { - Err.prototype = new Proto() - } - + Err.prototype = new Proto() return Err } -const LevelUPError = createError('LevelUPError') +const LevelUPError = createError('LevelUPError', Error) module.exports = { LevelUPError: LevelUPError, diff --git a/test.js b/test.js index da5fa54..3959aa4 100644 --- a/test.js +++ b/test.js @@ -3,11 +3,12 @@ const test = require('tape') const errors = require('.') -test('all errors are instances of LevelUPError', function (t) { +test('all errors are instances of Error and LevelUPError', function (t) { const LevelUPError = errors.LevelUPError const keys = Object.keys(errors) keys.forEach(function (key) { + t.ok(new errors[key]() instanceof Error) t.ok(new errors[key]() instanceof LevelUPError) }) From fa4c505bbb80eb37601f379ddd4d74575cf86a47 Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Sun, 19 Sep 2021 14:18:06 +0200 Subject: [PATCH 7/8] Use `t.is()` --- test.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test.js b/test.js index 3959aa4..7773e96 100644 --- a/test.js +++ b/test.js @@ -41,7 +41,7 @@ test('error with cause has expected properties', function (t) { t.is(error.type, 'ReadError') t.is(error.name, 'ReadError') - t.ok(error.cause === cause) + t.is(error.cause, cause) t.is(error.message, 'foo') t.end() }) @@ -52,7 +52,7 @@ test('error with message and cause has expected properties', function (t) { t.is(error.type, 'ReadError') t.is(error.name, 'ReadError') - t.ok(error.cause === cause) + t.is(error.cause, cause) t.is(error.message, 'bar') t.end() }) @@ -74,7 +74,7 @@ test('error message is writable for flexibility', function (t) { test('returns original instance if cause is the same type', function (t) { const cause = new errors.NotFoundError('Key not found in database [foo]') const error = new errors.NotFoundError(cause) - t.ok(cause === error, 'same instance') + t.is(cause, error, 'same instance') t.is(error.message, 'Key not found in database [foo]') t.end() }) @@ -82,7 +82,7 @@ test('returns original instance if cause is the same type', function (t) { test('returns new instance if cause prototype is different', function (t) { const cause = new errors.NotFoundError('Key not found in database [foo]') const error = new errors.WriteError(cause) - t.ok(cause !== error, 'new instance') + t.isNot(cause, error, 'new instance') t.is(error.message, 'Key not found in database [foo]') t.end() }) @@ -90,13 +90,13 @@ test('returns new instance if cause prototype is different', function (t) { test('returns original instance if message and cause are the same', function (t) { const cause = new errors.NotFoundError('Key not found in database [foo]') const error = new errors.NotFoundError('Key not found in database [foo]', cause) - t.ok(cause === error, 'same instance') + t.is(cause, error, 'same instance') t.end() }) test('returns new instance if message is different', function (t) { const cause = new errors.NotFoundError('Key not found in database [foo]') const error = new errors.NotFoundError('Key not found in database [bar]', cause) - t.ok(cause !== error, 'new instance') + t.isNot(cause, error, 'new instance') t.end() }) From 24c2c774401289364366a0bf45ef44e7a7a9ad14 Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Sun, 19 Sep 2021 14:19:58 +0200 Subject: [PATCH 8/8] Tweak test name to clarify `writable` choice --- test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test.js b/test.js index 7773e96..f0cff56 100644 --- a/test.js +++ b/test.js @@ -64,7 +64,7 @@ test('NotFoundError has special properties', function (t) { t.end() }) -test('error message is writable for flexibility', function (t) { +test('error message is writable to mirror node core conventions', function (t) { const error = new errors.WriteError('foo') error.message = 'Got error: ' + error.message t.is(error.message, 'Got error: foo')