Repository: pinojs/pino (js)

Issue:
Merge error flows instead of doing a special serialisation handling
I see you are planning to fix the error serialisation in #896 in the upcoming v7.

I submitted a bug a while ago #889 which addressed the loosing of the `message` property when a `msg` is present. Your recommendation was to use `{ err }`. I asked a question there but never got an answer.

I see in #896 you are detecting the error and running the serialiser manually. Why not just wrap the object in `{ err }` if it is an error and merge the 2 flows?

This will have the following advantages:
* less edge case programming
* `message` property is not lost
* logs with errors will be easily detected (especially in stuff like Elasticsearch)

Pull request description:
Merge error flows. (#923)
Merge the 2 error flows:
* if obj is of type Error wrap it in `{ err }`
* if msg not present set to error message (if present)

Update tests. Update API doc

Closes #923 

Patch:
diff --git a/docs/api.md b/docs/api.md
index 2951e21..fc32a94 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -476,6 +476,9 @@ logger.info({MIX: {IN: true}})
 // {"level":30,"time":1531254555820,"pid":55956,"hostname":"x","MIX":{"IN":true}}
 ```
 
+If the object is of type Error, it is wrapped in an object containing a property err (`{ err: mergingObject }`).
+This allows for a unified error handling flow.
+
 <a id="message"></a>
 #### `message` (String)
 
@@ -497,6 +500,9 @@ the `message` parameter not the value of the `msg` property on the `mergedObject
 See [Avoid Message Conflict](./help.md#avoid-message-conflict) for information
 on how to overcome this limitation.
 
+If no `message` parameter is provided, and the `mergedObject` is of type `Error` or it has a property named `err`, the 
+`message` parameter is set to the `message` value of the error.
+
 The `messageKey` option can be used at instantiation time to change the namespace
 from `msg` to another string as preferred.
 
diff --git a/lib/proto.js b/lib/proto.js
index 5e280e3..c2d9b81 100644
--- a/lib/proto.js
+++ b/lib/proto.js
@@ -148,15 +148,15 @@ function write (_obj, msg, num) {
   if (_obj === undefined || _obj === null) {
     obj = mixin ? mixin({}) : {}
   } else if (_obj instanceof Error) {
-    const serializers = this[serializersSym]
-    obj = Object.assign(mixin ? mixin(_obj) : {}, serializers.err(_obj))
-    // backward compat, set message
-    obj.message = undefined
-    if (!msg) {
+    obj = Object.assign(mixin ? mixin({}) : {}, { err: _obj })
+    if (msg === undefined) {
       msg = _obj.message
     }
   } else {
-    obj = Object.assign(mixin ? mixin(_obj) : {}, _obj)
+    obj = Object.assign(mixin ? mixin({}) : {}, _obj)
+    if (msg === undefined && _obj.err) {
+      msg = _obj.err.message
+    }
   }
 
   const s = this[asJsonSym](obj, msg, num, t)


Test patch:
diff --git a/test/basic.test.js b/test/basic.test.js
index 5625f14..df7f367 100644
--- a/test/basic.test.js
+++ b/test/basic.test.js
@@ -170,7 +170,8 @@ function levelTest (name, level) {
         type: 'Error',
         message: err.message,
         stack: err.stack
-      }
+      },
+      msg: err.message
     })
   })
 
diff --git a/test/error.test.js b/test/error.test.js
index dec0d7f..def0b22 100644
--- a/test/error.test.js
+++ b/test/error.test.js
@@ -25,10 +25,36 @@ test('err is serialized with additional properties set on the Error object', asy
     pid,
     hostname,
     level,
-    type: 'Error',
-    msg: err.message,
-    stack: err.stack,
-    foo: err.foo
+    err: {
+      type: 'Error',
+      message: err.message,
+      stack: err.stack,
+      foo: err.foo
+    },
+    msg: err.message
+  })
+})
+
+test('type should be detected based on constructor', async ({ ok, same }) => {
+  class Bar extends Error {}
+  const stream = sink()
+  const err = new Bar('myerror')
+  const instance = pino(stream)
+  instance.level = name
+  instance[name](err)
+  const result = await once(stream, 'data')
+  ok(new Date(result.time) <= new Date(), 'time is greater than Date.now()')
+  delete result.time
+  same(result, {
+    pid,
+    hostname,
+    level,
+    err: {
+      type: 'Bar',
+      message: err.message,
+      stack: err.stack
+    },
+    msg: err.message
   })
 })
 
@@ -46,10 +72,13 @@ test('type, message and stack should be first level properties', async ({ ok, sa
     pid,
     hostname,
     level,
-    type: 'Error',
-    msg: err.message,
-    stack: err.stack,
-    foo: err.foo
+    err: {
+      type: 'Error',
+      message: err.message,
+      stack: err.stack,
+      foo: err.foo
+    },
+    msg: err.message
   })
 })
 
@@ -76,7 +105,8 @@ test('err serializer', async ({ ok, same }) => {
       message: err.message,
       stack: err.stack,
       foo: err.foo
-    }
+    },
+    msg: err.message
   })
 })
 
@@ -95,10 +125,13 @@ test('an error with statusCode property is not confused for a http response', as
     pid,
     hostname,
     level,
-    type: 'Error',
-    msg: err.message,
-    stack: err.stack,
-    statusCode: err.statusCode
+    err: {
+      type: 'Error',
+      message: err.message,
+      stack: err.stack,
+      statusCode: err.statusCode
+    },
+    msg: err.message
   })
 })
 
@@ -124,8 +157,8 @@ test('stack is rendered as any other property if it\'s not a string', t => {
   var instance = pino(sink(function (chunk, enc, cb) {
     t.ok(new Date(chunk.time) <= new Date(), 'time is greater than Date.now()')
     delete chunk.time
-    t.equal(chunk.hasOwnProperty('stack'), true)
-    t.equal(chunk.stack, null)
+    t.equal(chunk.err.hasOwnProperty('stack'), true)
+    t.equal(chunk.err.stack, null)
     cb()
   }))
 
@@ -147,9 +180,12 @@ test('correctly ignores toString on errors', async ({ same }) => {
     pid,
     hostname,
     level: 60,
-    type: 'Error',
-    msg: err.message,
-    stack: err.stack
+    err: {
+      type: 'Error',
+      message: err.message,
+      stack: err.stack
+    },
+    msg: err.message
   })
 })
 
@@ -168,10 +204,13 @@ test('assign mixin()', async ({ same }) => {
     pid,
     hostname,
     level: 60,
-    type: 'Error',
-    msg: err.message,
-    stack: err.stack,
-    hello: 'world'
+    hello: 'world',
+    err: {
+      type: 'Error',
+      message: err.message,
+      stack: err.stack
+    },
+    msg: err.message
   })
 })
 
@@ -188,9 +227,78 @@ test('no err serializer', async ({ same }) => {
     pid,
     hostname,
     level: 60,
-    type: 'Error',
-    msg: err.message,
-    stack: err.stack
+    err: {
+      type: 'Error',
+      message: err.message,
+      stack: err.stack
+    },
+    msg: err.message
+  })
+})
+
+test('empty serializer', async ({ same }) => {
+  const err = new Error('myerror')
+  const stream = sink()
+  const instance = pino({
+    serializers: {
+      err () {}
+    }
+  }, stream)
+  instance.fatal(err)
+  const result = await once(stream, 'data')
+  delete result.time
+  same(result, {
+    pid,
+    hostname,
+    level: 60,
+    msg: err.message
+  })
+})
+
+test('assign mixin()', async ({ same }) => {
+  const err = new Error('myerror')
+  const stream = sink()
+  const instance = pino({
+    mixin () {
+      return { hello: 'world' }
+    }
+  }, stream)
+  instance.fatal(err)
+  const result = await once(stream, 'data')
+  delete result.time
+  same(result, {
+    pid,
+    hostname,
+    level: 60,
+    hello: 'world',
+    err: {
+      type: 'Error',
+      message: err.message,
+      stack: err.stack
+    },
+    msg: err.message
+  })
+})
+
+test('no err serializer', async ({ same }) => {
+  const err = new Error('myerror')
+  const stream = sink()
+  const instance = pino({
+    serializers: {}
+  }, stream)
+  instance.fatal(err)
+  const result = await once(stream, 'data')
+  delete result.time
+  same(result, {
+    pid,
+    hostname,
+    level: 60,
+    err: {
+      type: 'Error',
+      message: err.message,
+      stack: err.stack
+    },
+    msg: err.message
   })
 })
 
diff --git a/test/pretty.test.js b/test/pretty.test.js
index d88acf2..013454d 100644
--- a/test/pretty.test.js
+++ b/test/pretty.test.js
@@ -257,8 +257,8 @@ test('errors with props', async ({ isNot }) => {
   }))
   await once(child, 'close')
   isNot(strip(actual).match(/\(123456 on abcdefghijklmnopqr\): kaboom/), null)
-  isNot(strip(actual).match(/code: ENOENT/), null)
-  isNot(strip(actual).match(/errno: 1/), null)
+  isNot(strip(actual).match(/"code": "ENOENT"/), null)
+  isNot(strip(actual).match(/"errno": 1/), null)
   isNot(strip(actual).match(/.*error-props\.js.*/), null)
 })