Repository: pinojs/pino
Pull request #1067 (2021-07-12 06:55:32)

refactor: deprecate child bindings apply options
Deprecate apply options through bindings for child logger.

kibertoad: @climba03003 Was this intended for master and not next?
climba03003: > 
> 
> @climba03003 Was this intended for master and not next?

It should comes with #1059 and nothing is breaking. So, this PR can be landed on both `master` and `next` without problem.
kibertoad: @climba03003 next likely needs a slight TS types updates
climba03003: One of the fields is missing `bindings.level`. I am thinking of, how to test the deprecate warning.
kibertoad: @climba03003 How about putting a spy on console? might need an additional lib, though
climba03003: > 
> 
> doesn't this also need an assert to check the overall amount of asserts in test to ensure this is called?

`deprecations.test.js` is a tricky test files, it all rely on `pass` to prevent error. If we use the fixed test-case amount. It will becomes all failed. As the `onWarning` functions will receive all the other warning at the same time.

Patch:
diff --git a/benchmarks/child-creation.bench.js b/benchmarks/child-creation.bench.js
index 616e855..b09099f 100644
--- a/benchmarks/child-creation.bench.js
+++ b/benchmarks/child-creation.bench.js
@@ -60,6 +60,13 @@ const run = bench([
       child.info({ hello: 'world' })
     }
     setImmediate(cb)
+  },
+  function benchPinoCreationWithOption (cb) {
+    const child = plogDest.child({ a: 'property' }, { redact: [] })
+    for (var i = 0; i < max; i++) {
+      child.info({ hello: 'world' })
+    }
+    setImmediate(cb)
   }
 ], 10000)
 
diff --git a/browser.js b/browser.js
index 26a2c09..51989ba 100644
--- a/browser.js
+++ b/browser.js
@@ -113,13 +113,17 @@ function pino (opts) {
     set(setOpts, logger, 'trace', 'log')
   }
 
-  function child (bindings) {
+  function child (bindings, childOptions) {
     if (!bindings) {
       throw new Error('missing bindings for child Pino')
     }
-    const bindingsSerializers = bindings.serializers
-    if (serialize && bindingsSerializers) {
-      var childSerializers = Object.assign({}, serializers, bindingsSerializers)
+    childOptions = childOptions || {}
+    if (serialize && bindings.serializers) {
+      childOptions.serializers = bindings.serializers
+    }
+    const childOptionsSerializers = childOptions.serializers
+    if (serialize && childOptionsSerializers) {
+      var childSerializers = Object.assign({}, serializers, childOptionsSerializers)
       var childSerialize = opts.browser.serialize === true
         ? Object.keys(childSerializers)
         : serialize
diff --git a/docs/api.md b/docs/api.md
index 5437ed6..3f30471 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -680,24 +680,9 @@ const child = logger.child({foo: 'bar', level: 'debug'})
 child.debug('debug!') // will log as the `level` property set the level to debug
 ```
 
-##### `bindings.serializers` (Object)
+##### `bindings.serializers` (Object) - DEPRECATED
 
-Child loggers inherit the [serializers](#opt-serializers) from the parent logger.
-
-Setting the `serializers` key of the `bindings` object will override
-any configured parent serializers.
-
-```js
-const logger = require('pino')()
-logger.info({test: 'will appear'})
-// {"level":30,"time":1531259759482,"pid":67930,"hostname":"x","test":"will appear"}
-const child = logger.child({serializers: {test: () => `child-only serializer`}})
-child.info({test: 'will be overwritten'})
-// {"level":30,"time":1531259784008,"pid":67930,"hostname":"x","test":"child-only serializer"}
-```
-
-* See [`serializers` option](#opt-serializers)
-* See [pino.stdSerializers](#pino-stdSerializers)
+Use `options.serializers` instead.
 
 #### `options` (Object)
 
@@ -718,6 +703,25 @@ logger.info({ hello: 'world' })
 
 * See [`redact` option](#opt-redact)
 
+##### `options.serializers` (Object)
+
+Child loggers inherit the [serializers](#opt-serializers) from the parent logger.
+
+Setting the `serializers` key of the `options` object will override
+any configured parent serializers.
+
+```js
+const logger = require('pino')()
+logger.info({test: 'will appear'})
+// {"level":30,"time":1531259759482,"pid":67930,"hostname":"x","test":"will appear"}
+const child = logger.child({}, {serializers: {test: () => `child-only serializer`}})
+child.info({test: 'will be overwritten'})
+// {"level":30,"time":1531259784008,"pid":67930,"hostname":"x","test":"child-only serializer"}
+```
+
+* See [`serializers` option](#opt-serializers)
+* See [pino.stdSerializers](#pino-stdSerializers)
+
 <a id="bindings"></a>
 ### `logger.bindings()`
 
diff --git a/lib/proto.js b/lib/proto.js
index 4fd361d..a38e659 100644
--- a/lib/proto.js
+++ b/lib/proto.js
@@ -77,8 +77,6 @@ module.exports = function () {
 }
 
 const resetChildingsFormatter = bindings => bindings
-// TODO: formatters, serializers, customLevels should move to options
-// and depercation warning should emit
 function child (bindings, options) {
   if (!bindings) {
     throw Error('missing bindings for child Pino')
@@ -87,7 +85,24 @@ function child (bindings, options) {
   const serializers = this[serializersSym]
   const formatters = this[formattersSym]
   const instance = Object.create(this)
+
   if (bindings.hasOwnProperty('serializers') === true) {
+    process.emitWarning('bindings.serializers is deprecated, use options.serializers option instead', 'Warning', 'PINODEP004')
+    options.serializers = bindings.serializers
+  }
+  if (bindings.hasOwnProperty('formatters') === true) {
+    process.emitWarning('bindings.formatters is deprecated, use options.formatters option instead', 'Warning', 'PINODEP005')
+    options.formatters = bindings.formatters
+  }
+  if (bindings.hasOwnProperty('customLevels') === true) {
+    process.emitWarning('bindings.customLevels is deprecated, use options.customLevels option instead', 'Warning', 'PINODEP006')
+    options.customLevels = bindings.customLevels
+  }
+  if (bindings.hasOwnProperty('level') === true) {
+    process.emitWarning('bindings.level is deprecated, use options.level option instead', 'Warning', 'PINODEP007')
+    options.level = bindings.level
+  }
+  if (options.hasOwnProperty('serializers') === true) {
     instance[serializersSym] = Object.create(null)
 
     for (const k in serializers) {
@@ -100,17 +115,17 @@ function child (bindings, options) {
       instance[serializersSym][ks] = serializers[ks]
     }
 
-    for (const bk in bindings.serializers) {
-      instance[serializersSym][bk] = bindings.serializers[bk]
+    for (const bk in options.serializers) {
+      instance[serializersSym][bk] = options.serializers[bk]
     }
-    const bindingsSymbols = Object.getOwnPropertySymbols(bindings.serializers)
+    const bindingsSymbols = Object.getOwnPropertySymbols(options.serializers)
     for (var bi = 0; bi < bindingsSymbols.length; bi++) {
       const bks = bindingsSymbols[bi]
-      instance[serializersSym][bks] = bindings.serializers[bks]
+      instance[serializersSym][bks] = options.serializers[bks]
     }
   } else instance[serializersSym] = serializers
-  if (bindings.hasOwnProperty('formatters')) {
-    const { level, bindings: chindings, log } = bindings.formatters
+  if (options.hasOwnProperty('formatters')) {
+    const { level, bindings: chindings, log } = options.formatters
     instance[formattersSym] = buildFormatters(
       level || formatters.level,
       chindings || resetChildingsFormatter,
@@ -123,9 +138,9 @@ function child (bindings, options) {
       formatters.log
     )
   }
-  if (bindings.hasOwnProperty('customLevels') === true) {
-    assertNoLevelCollisions(this.levels, bindings.customLevels)
-    instance.levels = mappings(bindings.customLevels, instance[useOnlyCustomLevelsSym])
+  if (options.hasOwnProperty('customLevels') === true) {
+    assertNoLevelCollisions(this.levels, options.customLevels)
+    instance.levels = mappings(options.customLevels, instance[useOnlyCustomLevelsSym])
     genLsCache(instance)
   }
 
@@ -140,7 +155,7 @@ function child (bindings, options) {
   }
 
   instance[chindingsSym] = asChindings(instance, bindings)
-  const childLevel = bindings.level || this.level
+  const childLevel = options.level || this.level
   instance[setLevelSym](childLevel)
 
   return instance


Test patch:
diff --git a/test/browser-serializers.test.js b/test/browser-serializers.test.js
index 11d07ce..77d6dee 100644
--- a/test/browser-serializers.test.js
+++ b/test/browser-serializers.test.js
@@ -165,6 +165,32 @@ if (process.title !== 'browser') {
 
     console.error = consoleError
 
+    logger.child({
+      aBinding: 'test'
+    }, {
+      serializers: {
+        key: () => 'serialized',
+        key2: () => 'serialized2',
+        key3: () => 'serialized3'
+      }
+    }).fatal({ key: 'test' }, { key2: 'test' }, 'str should skip', [{ foo: 'array should skip' }], { key3: 'test' })
+    end()
+  })
+
+  test('serialize filter applies to child loggers through bindings', ({ end, is }) => {
+    const consoleError = console.error
+    console.error = function (binding, test, test2, test3, test4, test5) {
+      is(test.key, 'test')
+      is(test2.key2, 'serialized2')
+      is(test5.key3, 'test')
+    }
+
+    const logger = fresh('../browser', require)({
+      browser: { serialize: ['key2'] }
+    })
+
+    console.error = consoleError
+
     logger.child({
       aBinding: 'test',
       serializers: {
@@ -208,7 +234,8 @@ if (process.title !== 'browser') {
     console.error = consoleError
 
     logger.child({
-      key: 'test',
+      key: 'test'
+    }, {
       serializers: {
         key: () => 'serialized'
       }
@@ -233,7 +260,7 @@ test('child does not overwrite parent serializers', ({ end, is }) => {
       }
     }
   })
-  const child = parent.child({ serializers: childSerializers })
+  const child = parent.child({}, { serializers: childSerializers })
 
   parent.fatal({ test: 'test' })
   child.fatal({ test: 'test' })
@@ -266,7 +293,7 @@ test('children serializers get called', ({ end, is }) => {
     }
   })
 
-  const child = parent.child({ a: 'property', serializers: childSerializers })
+  const child = parent.child({ a: 'property' }, { serializers: childSerializers })
 
   child.fatal({ test: 'test' })
   end()
@@ -284,7 +311,7 @@ test('children serializers get called when inherited from parent', ({ end, is })
     }
   })
 
-  const child = parent.child({ serializers: { test: () => 'pass' } })
+  const child = parent.child({}, { serializers: { test: () => 'pass' } })
 
   child.fatal({ test: 'fail' })
   end()
@@ -317,7 +344,7 @@ test('non overridden serializers are available in the children', ({ end, is }) =
     }
   })
 
-  const child = parent.child({ serializers: cSerializers })
+  const child = parent.child({}, { serializers: cSerializers })
 
   child.fatal({ shared: 'test' })
   child.fatal({ onlyParent: 'test' })
diff --git a/test/custom-levels.test.js b/test/custom-levels.test.js
index 194f857..ecae49a 100644
--- a/test/custom-levels.test.js
+++ b/test/custom-levels.test.js
@@ -98,10 +98,11 @@ test('custom levels are inherited by children', async ({ equal }) => {
 test('custom levels can be specified on child bindings', async ({ equal }) => {
   const stream = sink()
   const logger = pino(stream).child({
+    childMsg: 'ok'
+  }, {
     customLevels: {
       foo: 35
-    },
-    childMsg: 'ok'
+    }
   })
 
   logger.foo('test')
@@ -114,10 +115,11 @@ test('custom levels can be specified on child bindings', async ({ equal }) => {
 test('customLevels property child bindings does not get logged', async ({ equal }) => {
   const stream = sink()
   const logger = pino(stream).child({
+    childMsg: 'ok'
+  }, {
     customLevels: {
       foo: 35
-    },
-    childMsg: 'ok'
+    }
   })
 
   logger.foo('test')
@@ -131,7 +133,7 @@ test('throws when specifying pre-existing parent labels via child bindings', asy
     customLevels: {
       foo: 35
     }
-  }, stream).child({
+  }, stream).child({}, {
     customLevels: {
       foo: 45
     }
@@ -144,7 +146,7 @@ test('throws when specifying pre-existing parent values via child bindings', asy
     customLevels: {
       foo: 35
     }
-  }, stream).child({
+  }, stream).child({}, {
     customLevels: {
       bar: 35
     }
@@ -153,7 +155,7 @@ test('throws when specifying pre-existing parent values via child bindings', asy
 
 test('throws when specifying core values via child bindings', async ({ throws }) => {
   const stream = sink()
-  throws(() => pino(stream).child({
+  throws(() => pino(stream).child({}, {
     customLevels: {
       foo: 30
     }
@@ -199,11 +201,11 @@ test('custom level below level threshold will not log', async ({ equal }) => {
 test('does not share custom level state across siblings', async ({ doesNotThrow }) => {
   const stream = sink()
   const logger = pino(stream)
-  logger.child({
+  logger.child({}, {
     customLevels: { foo: 35 }
   })
   doesNotThrow(() => {
-    logger.child({
+    logger.child({}, {
       customLevels: { foo: 35 }
     })
   })
diff --git a/test/deprecations.test.js b/test/deprecations.test.js
index 5b8cec1..ce0fb46 100644
--- a/test/deprecations.test.js
+++ b/test/deprecations.test.js
@@ -1,6 +1,7 @@
 'use strict'
 /* eslint no-prototype-builtins: 0 */
 
+const { hostname } = require('os')
 const { test } = require('tap')
 const { sink, once } = require('./helper')
 const pino = require('../')
@@ -99,3 +100,143 @@ test('pino.* serializer', async ({ match, equal, pass }) => {
   match(await o, { level: 30 })
   process.removeListener('warning', onWarning)
 })
+
+test('child(bindings.serializers)', async ({ match, equal, pass }) => {
+  process.on('warning', onWarning)
+  function onWarning (warn) {
+    equal(warn.code, 'PINODEP004')
+  }
+
+  const stream = sink()
+  const parent = pino({ serializers: { test: () => 'parent' } }, stream)
+  const child = parent.child({
+    serializers: {
+      test () {
+        pass('called')
+        return 'child'
+      }
+    }
+  })
+
+  const o = once(stream, 'data')
+  child.fatal({ test: 'test' })
+  match(await o, { test: 'child' })
+  process.removeListener('warning', onWarning)
+})
+
+test('child(bindings.formatters)', async ({ match, equal, pass }) => {
+  process.on('warning', onWarning)
+  function onWarning (warn) {
+    equal(warn.code, 'PINODEP005')
+  }
+
+  const stream = sink()
+  const logger = pino({
+    formatters: {
+      level (label, number) {
+        return {
+          log: {
+            level: label
+          }
+        }
+      },
+      bindings (bindings) {
+        return {
+          process: {
+            pid: bindings.pid
+          },
+          host: {
+            name: bindings.hostname
+          }
+        }
+      },
+      log (obj) {
+        return { hello: 'world', ...obj }
+      }
+    }
+  }, stream)
+
+  const child = logger.child({
+    foo: 'bar',
+    nested: { object: true },
+    formatters: {
+      bindings (bindings) {
+        pass('called')
+        return { ...bindings, faz: 'baz' }
+      }
+    }
+  })
+
+  const o = once(stream, 'data')
+  child.info('hello world')
+  match(await o, {
+    log: {
+      level: 'info'
+    },
+    process: {
+      pid: process.pid
+    },
+    host: {
+      name: hostname()
+    },
+    hello: 'world',
+    foo: 'bar',
+    nested: { object: true },
+    faz: 'baz'
+  })
+  process.removeListener('warning', onWarning)
+})
+
+test('child(bindings.customLevels)', async ({ match, equal, pass }) => {
+  process.on('warning', onWarning)
+  function onWarning (warn) {
+    equal(warn.code, 'PINODEP006')
+  }
+
+  const stream = sink()
+  const logger = pino(stream).child({
+    childMsg: 'ok',
+    customLevels: {
+      foo: 35
+    }
+  }, {
+    formatters: {
+      level (label, number) {
+        if (label === 'foo' && number === 35) {
+          pass('using customLevels')
+        }
+        return { level: number }
+      }
+    }
+  })
+
+  const o = once(stream, 'data')
+  logger.foo('test')
+  match(await o, {
+    level: 35,
+    childMsg: 'ok',
+    msg: 'test'
+  })
+  process.removeListener('warning', onWarning)
+})
+
+test('child(bindings.level)', async ({ equal, pass }) => {
+  process.on('warning', onWarning)
+  function onWarning (warn) {
+    equal(warn.code, 'PINODEP007')
+  }
+
+  const stream = sink()
+  const logger = pino({
+    level: 'info'
+  }, stream).child({
+    level: 'trace'
+  })
+
+  const o = once(stream, 'data')
+  logger.info('test')
+  if (await o === null) {
+    pass('child can overrid parent level')
+  }
+  process.removeListener('warning', onWarning)
+})
diff --git a/test/formatters.test.js b/test/formatters.test.js
index e5afb4d..7385999 100644
--- a/test/formatters.test.js
+++ b/test/formatters.test.js
@@ -170,7 +170,8 @@ test('Formatters in child logger', async ({ match }) => {
 
   const child = logger.child({
     foo: 'bar',
-    nested: { object: true },
+    nested: { object: true }
+  }, {
     formatters: {
       bindings (bindings) {
         return { ...bindings, faz: 'baz' }
@@ -226,7 +227,8 @@ test('Formatters without bindings in child logger', async ({ match }) => {
 
   const child = logger.child({
     foo: 'bar',
-    nested: { object: true },
+    nested: { object: true }
+  }, {
     formatters: {
       log (obj) {
         return { other: 'stuff', ...obj }
diff --git a/test/serializers.test.js b/test/serializers.test.js
index af12d3a..5655be7 100644
--- a/test/serializers.test.js
+++ b/test/serializers.test.js
@@ -70,7 +70,7 @@ test('undefined overrides default err namespace error serializer', async ({ equa
 test('serializers override values', async ({ equal }) => {
   const stream = sink()
   const parent = pino({ serializers: parentSerializers }, stream)
-  parent.child({ serializers: childSerializers })
+  parent.child({}, { serializers: childSerializers })
 
   parent.fatal({ test: 'test' })
   const o = await once(stream, 'data')
@@ -80,7 +80,7 @@ test('serializers override values', async ({ equal }) => {
 test('child does not overwrite parent serializers', async ({ equal }) => {
   const stream = sink()
   const parent = pino({ serializers: parentSerializers }, stream)
-  const child = parent.child({ serializers: childSerializers })
+  const child = parent.child({}, { serializers: childSerializers })
 
   parent.fatal({ test: 'test' })
 
@@ -99,7 +99,7 @@ test('Symbol.for(\'pino.serializers\')', async ({ equal, not }) => {
   equal(parent[Symbol.for('pino.serializers')], parentSerializers)
   equal(child[Symbol.for('pino.serializers')], parentSerializers)
 
-  const child2 = parent.child({
+  const child2 = parent.child({}, {
     serializers: {
       a
     }
@@ -133,7 +133,7 @@ test('children inherit parent Symbol serializers', async ({ equal, not }) => {
 
   equal(parent[Symbol.for('pino.serializers')], symbolSerializers)
 
-  const child = parent.child({
+  const child = parent.child({}, {
     serializers: {
       [Symbol.for('a')]: a,
       a
@@ -156,7 +156,7 @@ test('children serializers get called', async ({ equal }) => {
     test: 'this'
   }, stream)
 
-  const child = parent.child({ a: 'property', serializers: childSerializers })
+  const child = parent.child({ a: 'property' }, { serializers: childSerializers })
 
   child.fatal({ test: 'test' })
   const o = await once(stream, 'data')
@@ -170,7 +170,7 @@ test('children serializers get called when inherited from parent', async ({ equa
     serializers: parentSerializers
   }, stream)
 
-  const child = parent.child({ serializers: { test: function () { return 'pass' } } })
+  const child = parent.child({}, { serializers: { test: function () { return 'pass' } } })
 
   child.fatal({ test: 'fail' })
   const o = await once(stream, 'data')
@@ -191,7 +191,7 @@ test('non-overridden serializers are available in the children', async ({ equal
 
   const parent = pino({ serializers: pSerializers }, stream)
 
-  const child = parent.child({ serializers: cSerializers })
+  const child = parent.child({}, { serializers: cSerializers })
 
   const o = once(stream, 'data')
   child.fatal({ shared: 'test' })