fix(levels): useLevelLabels permanently mutates initial cache

[pgoldrbx]

Issue

Once a logger instance is created with useLevelLabels: true this can never be undone.

• All loggers using this module will update to use level labels.
• Creating new instances without the option specified will not use the default value, but will continue to use labels.
• Creating a logger useLevelLabels: false will have no effect
• NOTE: any pino instances created by child packages, using incompatible semver (and thus a different/dup package) will not be impacted by this, and will continue to operate independently

Opinion

I can see some benefit in keeping the log records appearing uniformly everywhere. However, I might expect that to be a static method, or otherwise very explicitly documented. Since this is documented as a constructor-level option, I expect it to behave accordingly.

Root Cause

An initial cache of levels (as numbers) is created when the package is required. This is assigned to the instance prototype. When useLevelLabels is true, genCache is called to update the instance level display labels. This modifies the instance property, stored on the prototype, which points to the cached, initial object.

Solution

As I see it, there are two ways to approach this:

Generate the prototype property

My preferred solution would be to prevent ever allowing the defaults to be mutable but wrapping them in a getter function. Instead of assigning initialLsCache directly, we would call getInitialLsCache to get a copy, and preserve the original.

const prototype = {
 // ...
  [lsCacheSym]: getInitialLsCache(),

However, this would be executed for every Pino instance created. This library has been optimized for performance, so this is probably not the preferred solution.

Update the instance with a copy

Since this is an uncommon use case, we want to avoid adding extra complexity into the commonly used code paths. Instead, we'll make a shallow copy of the initial cache object when we need to make changes. We can do this in the existing genLsCache method.

instance[lsCacheSym] = Object.assign({}, instance[lsCacheSym])

(or use Object.assign directly with the reduce initial value)

This code will only be executed in the cases when it is needed. It prioritizes overall performance over cache integrity.

[pgoldrbx]

I'm trying to figure out why coverage is reporting a missing branch. I feel like the "else" case should be well covered.

Edit: coverage tools are great. the else case can't be reached, and so my solution can be simplified. We don't need to check against the initial value. We can just always perform a shallow copy.

[jsumners]

LGTM

[mcollina]

LGTM

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.