From ea2d76de5113b54771f8b706ff3be102d6a3db6f Mon Sep 17 00:00:00 2001 From: John Lees-Miller Date: Fri, 11 Sep 2020 10:05:54 +0100 Subject: [PATCH] Improve tagging for a (maybe) null error (#12) * Document a way of handling an error that might be null * Make typecheck strict * Test some edge cases Co-authored-by: Jakob Ackermann --- libraries/o-error/README.md | 27 +++++++++++++- libraries/o-error/index.d.ts | 26 ++++++++++++-- libraries/o-error/index.js | 36 ++++++++++++++++--- libraries/o-error/package-lock.json | 6 ++++ libraries/o-error/package.json | 3 +- libraries/o-error/test/o-error-util.test.js | 39 +++++++++++++++++++++ libraries/o-error/test/support/index.js | 16 ++++++--- 7 files changed, 140 insertions(+), 13 deletions(-) diff --git a/libraries/o-error/README.md b/libraries/o-error/README.md index c7839174bb..7ecea2dc76 100644 --- a/libraries/o-error/README.md +++ b/libraries/o-error/README.md @@ -366,7 +366,7 @@ Set the extra info object for this error. | Param | Type | Description | | --- | --- | --- | -| info | Object \| null \| undefined | extra data to attach to the error | +| info | Object | extra data to attach to the error | @@ -394,6 +394,31 @@ return it. | [message] | string | message with which to tag `error` | | [info] | Object | extra data with wich to tag `error` | +**Example** *(An error in a callback)* +```js +function findUser(name, callback) { + fs.readFile('/etc/passwd', (err, data) => { + if (err) return callback(OError.tag(err, 'failed to read passwd')) + // ... + }) +} +``` +**Example** *(A possible error in a callback)* +```js +function cleanup(callback) { + fs.unlink('/tmp/scratch', (err) => callback(err && OError.tag(err))) +} +``` +**Example** *(An error with async/await)* +```js +async function cleanup() { + try { + await fs.promises.unlink('/tmp/scratch') + } catch (err) { + throw OError.tag(err, 'failed to remove scratch file') + } +} +``` ### OError.getFullInfo(error) ⇒ Object diff --git a/libraries/o-error/index.d.ts b/libraries/o-error/index.d.ts index 000448e49f..6d46db33ec 100644 --- a/libraries/o-error/index.d.ts +++ b/libraries/o-error/index.d.ts @@ -8,6 +8,28 @@ declare class OError extends Error { * Tag debugging information onto any error (whether an OError or not) and * return it. * + * @example An error in a callback + * function findUser(name, callback) { + * fs.readFile('/etc/passwd', (err, data) => { + * if (err) return callback(OError.tag(err, 'failed to read passwd')) + * // ... + * }) + * } + * + * @example A possible error in a callback + * function cleanup(callback) { + * fs.unlink('/tmp/scratch', (err) => callback(err && OError.tag(err))) + * } + * + * @example An error with async/await + * async function cleanup() { + * try { + * await fs.promises.unlink('/tmp/scratch') + * } catch (err) { + * throw OError.tag(err, 'failed to remove scratch file') + * } + * } + * * @param {Error} error the error to tag * @param {string} [message] message with which to tag `error` * @param {Object} [info] extra data with wich to tag `error` @@ -39,12 +61,12 @@ declare class OError extends Error { constructor(message: string, info?: any, cause?: Error); info: any; cause: Error; - /** @private @type {Array} */ + /** @private @type {Array | undefined} */ private _oErrorTags; /** * Set the extra info object for this error. * - * @param {Object | null | undefined} info extra data to attach to the error + * @param {Object} info extra data to attach to the error * @return {this} */ withInfo(info: any): OError; diff --git a/libraries/o-error/index.js b/libraries/o-error/index.js index 1184fcfe35..515e39574c 100644 --- a/libraries/o-error/index.js +++ b/libraries/o-error/index.js @@ -13,15 +13,14 @@ class OError extends Error { this.name = this.constructor.name if (info) this.info = info if (cause) this.cause = cause - - /** @private @type {Array} */ + /** @private @type {Array | undefined} */ this._oErrorTags // eslint-disable-line } /** * Set the extra info object for this error. * - * @param {Object | null | undefined} info extra data to attach to the error + * @param {Object} info extra data to attach to the error * @return {this} */ withInfo(info) { @@ -44,6 +43,28 @@ class OError extends Error { * Tag debugging information onto any error (whether an OError or not) and * return it. * + * @example An error in a callback + * function findUser(name, callback) { + * fs.readFile('/etc/passwd', (err, data) => { + * if (err) return callback(OError.tag(err, 'failed to read passwd')) + * // ... + * }) + * } + * + * @example A possible error in a callback + * function cleanup(callback) { + * fs.unlink('/tmp/scratch', (err) => callback(err && OError.tag(err))) + * } + * + * @example An error with async/await + * async function cleanup() { + * try { + * await fs.promises.unlink('/tmp/scratch') + * } catch (err) { + * throw OError.tag(err, 'failed to remove scratch file') + * } + * } + * * @param {Error} error the error to tag * @param {string} [message] message with which to tag `error` * @param {Object} [info] extra data with wich to tag `error` @@ -60,7 +81,7 @@ class OError extends Error { tag = /** @type TaggedError */ ({ name: 'TaggedError', message, info }) Error.captureStackTrace(tag, OError.tag) } else { - tag = new TaggedError(message, info) + tag = new TaggedError(message || '', info) } oError._oErrorTags.push(tag) @@ -106,7 +127,7 @@ class OError extends Error { const oError = /** @type{OError} */ (error) - let stack = oError.stack + let stack = oError.stack || '(no stack)' if (Array.isArray(oError._oErrorTags) && oError._oErrorTags.length) { stack += `\n${oError._oErrorTags.map((tag) => tag.stack).join('\n')}` @@ -129,6 +150,11 @@ class OError extends Error { */ class TaggedError extends OError {} +/** + * @private + * @param {string} string + * @return {string} + */ function indent(string) { return string.replace(/^/gm, ' ') } diff --git a/libraries/o-error/package-lock.json b/libraries/o-error/package-lock.json index bc02939ca8..6104ad9a0c 100644 --- a/libraries/o-error/package-lock.json +++ b/libraries/o-error/package-lock.json @@ -317,6 +317,12 @@ "integrity": "sha512-tsAQNx32a8CoFhjhijUIhI4kccIAgmGhy8LZMZgGfmXcpMbPRUqn5LWmgRttILi6yeGmBJd2xsPkFMs0PzgPCw==", "dev": true }, + "@types/chai": { + "version": "4.2.12", + "resolved": "https://registry.npmjs.org/@types/chai/-/chai-4.2.12.tgz", + "integrity": "sha512-aN5IAC8QNtSUdQzxu7lGBgYAOuU1tmRU4c9dIq5OKGf/SBVjXo+ffM2wEjudAWbgpOhy60nLoAGH1xm8fpCKFQ==", + "dev": true + }, "@types/color-name": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/@types/color-name/-/color-name-1.1.1.tgz", diff --git a/libraries/o-error/package.json b/libraries/o-error/package.json index 9f21f7e10b..ee258c21af 100644 --- a/libraries/o-error/package.json +++ b/libraries/o-error/package.json @@ -22,7 +22,7 @@ "update-readme": "doc/update-readme.js", "test": "mocha", "test:coverage": "nyc --reporter=lcov --reporter=text-summary npm run test", - "typecheck": "tsc --allowJs --checkJs --noEmit --moduleResolution node --target ES6 *.js test/**/*.js", + "typecheck": "tsc --allowJs --checkJs --noEmit --moduleResolution node --strict --target ES6 *.js test/**/*.js", "declaration:build": "rm -f index.d.ts && tsc --allowJs --declaration --emitDeclarationOnly --moduleResolution node --target ES6 index.js", "declaration:check": "git diff --exit-code -- index.d.ts", "prepublishOnly": "npm run --silent declaration:build && npm run --silent declaration:check" @@ -31,6 +31,7 @@ "license": "MIT", "repository": "github:overleaf/o-error", "devDependencies": { + "@types/chai": "^4.2.12", "@types/node": "^13.13.2", "chai": "^3.3.0", "eslint": "^6.8.0", diff --git a/libraries/o-error/test/o-error-util.test.js b/libraries/o-error/test/o-error-util.test.js index dd7d28654d..28f26354f0 100644 --- a/libraries/o-error/test/o-error-util.test.js +++ b/libraries/o-error/test/o-error-util.test.js @@ -149,9 +149,44 @@ describe('OError.tag', function () { expect.fail('should have yielded an error') }) }) + + it('is not included in the stack trace if using capture', function () { + if (!Error.captureStackTrace) return this.skip() + const err = new Error('test error') + OError.tag(err, 'test message') + const stack = OError.getFullStack(err) + expect(stack).to.match(/TaggedError: test message\n\s+at/) + expect(stack).to.not.match(/TaggedError: test message\n\s+at [\w.]*tag/) + }) + + describe('without Error.captureStackTrace', function () { + /* eslint-disable mocha/no-hooks-for-single-case */ + before(function () { + this.originalCaptureStackTrace = Error.captureStackTrace + Error.captureStackTrace = null + }) + after(function () { + Error.captureStackTrace = this.originalCaptureStackTrace + }) + + it('still captures a stack trace, albeit including itself', function () { + const err = new Error('test error') + OError.tag(err, 'test message') + expectFullStackWithoutStackFramesToEqual(err, [ + 'Error: test error', + 'TaggedError: test message', + ]) + const stack = OError.getFullStack(err) + expect(stack).to.match(/TaggedError: test message\n\s+at [\w.]*tag/) + }) + }) }) describe('OError.getFullInfo', function () { + it('works when given null', function () { + expect(OError.getFullInfo(null)).to.deep.equal({}) + }) + it('works on a normal error', function () { const err = new Error('foo') expect(OError.getFullInfo(err)).to.deep.equal({}) @@ -193,6 +228,10 @@ describe('OError.getFullInfo', function () { }) describe('OError.getFullStack', function () { + it('works when given null', function () { + expect(OError.getFullStack(null)).to.equal('') + }) + it('works on a normal error', function () { const err = new Error('foo') const fullStack = OError.getFullStack(err) diff --git a/libraries/o-error/test/support/index.js b/libraries/o-error/test/support/index.js index 46dd49967b..34308c5449 100644 --- a/libraries/o-error/test/support/index.js +++ b/libraries/o-error/test/support/index.js @@ -2,6 +2,10 @@ const { expect } = require('chai') const OError = require('../..') +/** + * @param {Error} e + * @param {any} expected + */ exports.expectError = function OErrorExpectError(e, expected) { expect( e.name, @@ -23,24 +27,28 @@ exports.expectError = function OErrorExpectError(e, expected) { 'error should be recognised by util.types.isNativeError' ).to.be.true - expect(e.stack, 'error should have a stack trace').to.be.truthy - expect( e.toString(), 'toString should return the default error message formatting' ).to.equal(expected.message) + expect(e.stack, 'error should have a stack trace').to.not.be.empty + expect( - e.stack.split('\n')[0], + /** @type {string} */ (e.stack).split('\n')[0], 'stack should start with the default error message formatting' ).to.match(new RegExp(`^${expected.name}:`)) expect( - e.stack.split('\n')[1], + /** @type {string} */ (e.stack).split('\n')[1], 'first stack frame should be the function where the error was thrown' ).to.match(expected.firstFrameRx) } +/** + * @param {Error} error + * @param {String[]} expected + */ exports.expectFullStackWithoutStackFramesToEqual = function (error, expected) { const fullStack = OError.getFullStack(error) const fullStackWithoutFrames = fullStack