Skip to content

Commit

Permalink
fixes bug which dropped non-enumerable fields from 'data' argument (#22)
Browse files Browse the repository at this point in the history
This commit removes the use of `_.cloneDeep` on additional context
passed to a `log` call (third positional argument).

Cloning `data` introduced a breaking change for individuals passing
objects with non-enumerable properties as the `data` argument.

**Note:**

This reversion re-introduces the risk of mutation when paired with
plug-ins. It also restores the built-in mutation that will append a
`._label` attribute to any `data` argument.

The README was updated to reflect those risks and explain how
they can be managed through careful usage.
  • Loading branch information
mattoberle authored Nov 17, 2020
1 parent b1826b7 commit 3e951ba
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 21 deletions.
25 changes: 25 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,31 @@ The arguments are in the following order:
2. Message (String)
3. Accompanying log information (Object)

### Safety Considerations :warning:

The safest way to use the third position argument (`data`) is to wrap an object
rather than pass an object directly. The object provided should have enumerable properties.

Passing an object directly may result in a thrown error (if it's not really an object).
```js
const obj = 'string';
log('info', 'this is unsafe', obj); // throws
```

Passing an object directly may result in mutation. Be mindful of this when utilizing plug-ins.
```js
const obj = { key: 'value' };
log('info', 'this is unsafe', obj);
obj._label // 'INFO'
```

These are safer ways to provide the `data` argument.
```js
log('info', 'this is safer', { obj });
log('info', 'this is safer', { ...obj });
log('error', 'this is safer', { stack: err.stack });
```

### Setting Minimum Log Level

For production instances it may not be necessary to log the same messages you do in dev environments. By default, Clay Log will only display `info` and above level logs, but this can be configured with an environment variable. Set `process.env.LOG` to a [corresponding log level](#usage) and that will be the minimum level that appears.
Expand Down
7 changes: 3 additions & 4 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict';

const cloneDeep = require('lodash.clonedeep'),
isNode = typeof process !== 'undefined'
const isNode = typeof process !== 'undefined'
&& process.versions != null
&& process.versions.node != null;

Expand Down Expand Up @@ -173,8 +172,8 @@ function log(instanceLog) {
instanceLog = initPlugins()(instanceLog);
}

return function (level, msg, data = {}) {
data = cloneDeep(data);
return function (level, msg, data) {
data = data || {};

if (level instanceof Error) {
msg = level;
Expand Down
5 changes: 0 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
"homepage": "https://github.com/clay/clay-log#readme",
"dependencies": {
"ansi-styles": "^3.2.0",
"lodash.clonedeep": "^4.5.0",
"pino": "^4.7.1"
},
"devDependencies": {
Expand Down
12 changes: 1 addition & 11 deletions test.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ describe(dirname, function () {

log('info', 'message', data);
sinon.assert.calledOnce(fakeLogInstance.info);
sinon.assert.calledWith(fakeLogInstance.info, { _label: 'INFO', some: 'data' }, 'message');
sinon.assert.calledWith(fakeLogInstance.info, data, 'message');
});


Expand Down Expand Up @@ -221,16 +221,6 @@ describe(dirname, function () {
sinon.assert.calledWith(fakeLogInstance.info._original, expected, 'message');
});

it('does not mutate the object passed to the logger', function () {
process.env.CLAY_LOG_PLUGINS = 'heap';
const fakeLogInstance = createFakeLogger()(),
log = fn(fakeLogInstance),
data = { some: 'data' };

log('info', 'message', data);
expect(data).to.deep.equal({ some: 'data' });
});

it('doesn\'t log memory usage if CLAY_LOG_PLUGINS does not contain "heap"', function () {
process.env.CLAY_LOG_PLUGINS = '';
const fakeLogInstance = createFakeLogger()(),
Expand Down

0 comments on commit 3e951ba

Please sign in to comment.