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 <jakob.ackermann@overleaf.com>

libraries/o-error/README.md

@@ -366,7 +366,7 @@ Set the extra info object for this error.
 
 | Param | Type | Description |
 | --- | --- | --- |
-| info | <code>Object</code> \| <code>null</code> \| <code>undefined</code> | extra data to attach to the error |
+| info | <code>Object</code> | extra data to attach to the error |
 
 <a name="OError+withCause"></a>
 
@@ -394,6 +394,31 @@ return it.
 | [message] | <code>string</code> | message with which to tag `error` |
 | [info] | <code>Object</code> | 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')
+  }
+}
+```
 <a name="OError.getFullInfo"></a>
 
 ### OError.getFullInfo(error) ⇒ <code>Object</code>

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 <caption>An error in a callback</caption>
+     * function findUser(name, callback) {
+     *   fs.readFile('/etc/passwd', (err, data) => {
+     *     if (err) return callback(OError.tag(err, 'failed to read passwd'))
+     *     // ...
+     *   })
+     * }
+     *
+     * @example <caption>A possible error in a callback</caption>
+     * function cleanup(callback) {
+     *   fs.unlink('/tmp/scratch', (err) => callback(err && OError.tag(err)))
+     * }
+     *
+     * @example <caption>An error with async/await</caption>
+     * 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<TaggedError>} */
+    /** @private @type {Array<TaggedError> | 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;

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<TaggedError> | undefined} */
-    /** @private @type {Array<TaggedError>} */
     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 <caption>An error in a callback</caption>
+   * function findUser(name, callback) {
+   *   fs.readFile('/etc/passwd', (err, data) => {
+   *     if (err) return callback(OError.tag(err, 'failed to read passwd'))
+   *     // ...
+   *   })
+   * }
+   *
+   * @example <caption>A possible error in a callback</caption>
+   * function cleanup(callback) {
+   *   fs.unlink('/tmp/scratch', (err) => callback(err && OError.tag(err)))
+   * }
+   *
+   * @example <caption>An error with async/await</caption>
+   * 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, '    ')
 }

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",

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",

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)

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