Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: change inspect depth default #17907

Closed
wants to merge 7 commits into from
Closed

util: change inspect depth default #17907

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
94b7b01
streams: add custom inspect to BufferList
BridgeAR Dec 30, 2017
2d845cf
util: change util.inspect depth default
BridgeAR Dec 28, 2017
2721767
util: improve setting default options
BridgeAR Dec 28, 2017
606f06c
fixup address comments
BridgeAR Jan 2, 2018
3ca34ca
fixup update documentation
BridgeAR Jan 3, 2018
8ab8726
util: change %o depth default
BridgeAR Jan 3, 2018
e220598
fixup
BridgeAR Jan 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/internal/streams/BufferList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
'use strict';

const { Buffer } = require('buffer');
const { customInspectSymbol } = require('internal/util');
const util = require('util');

function copyBuffer(src, target, offset) {
Buffer.prototype.copy.call(src, target, offset);
Expand Down Expand Up @@ -73,4 +75,9 @@ module.exports = class BufferList {
}
return ret;
}

[customInspectSymbol]() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d just use util.inspect.custom here, it saves people looking at this code one round-trip to another file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

const obj = util.inspect({ length: this.length });
return `${this.constructor.name} ${obj}`;
}
};
5 changes: 3 additions & 2 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,9 @@ function hasOwnProperty(obj, prop) {
// Can overridden with custom print functions, such as `probe` or `eyes.js`.
// This is the default "writer" value if none is passed in the REPL options.
const writer = exports.writer = (obj) => util.inspect(obj, writer.options);
writer.options =
Object.assign({}, util.inspect.defaultOptions, { showProxy: true });
writer.options = Object.assign({},
util.inspect.defaultOptions,
{ showProxy: true, depth: 2 });

exports._builtinLibs = internalModule.builtinLibs;

Expand Down
5 changes: 2 additions & 3 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const {

const inspectDefaultOptions = Object.seal({
showHidden: false,
depth: 2,
depth: null,
colors: false,
customInspect: true,
showProxy: false,
Expand Down Expand Up @@ -317,8 +317,7 @@ Object.defineProperty(inspect, 'defaultOptions', {
if (options === null || typeof options !== 'object') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'Object');
}
Object.assign(inspectDefaultOptions, options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not using _extend?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type check has to be done again in that case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt it will add a noticeable overhead but yes it makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Do we actually care about the performance of inspection? I thought we didn’t.
  2. Same concern here as @lpinca. This is quite literally reinventing the wheel, and type checks for object are quite fast in modern V8.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the use _extend as it will still be faster than Object.assign

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the performance - just recently I heard that inspect came up in a flamegraph. Seems like people use it a lot. I am aware that setting the default options will not be a big thing but the first implementation (not changing inspect for console.*) used it a lot and that is why I decided to optimize it as well.

return inspectDefaultOptions;
return _extend(inspectDefaultOptions, options);
}
});

Expand Down
8 changes: 8 additions & 0 deletions test/parallel/test-stream-buffer-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require('../common');
const assert = require('assert');
const BufferList = require('internal/streams/BufferList');
const util = require('util');

// Test empty buffer list.
const emptyList = new BufferList();
Expand All @@ -25,3 +26,10 @@ assert.strictEqual(list.join(','), 'foo');
const shifted = list.shift();
assert.strictEqual(shifted, 'foo');
assert.deepStrictEqual(list, new BufferList());

const tmp = util.inspect.defaultOptions.colors;
util.inspect.defaultOptions = { colors: true };
assert.strictEqual(
util.inspect(list),
'BufferList { length: \u001b[33m0\u001b[39m }');
util.inspect.defaultOptions = { colors: tmp };
16 changes: 10 additions & 6 deletions test/parallel/test-util-inspect-proxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,17 @@ const expected1 = 'Proxy [ {}, {} ]';
const expected2 = 'Proxy [ Proxy [ {}, {} ], {} ]';
const expected3 = 'Proxy [ Proxy [ Proxy [ {}, {} ], {} ], Proxy [ {}, {} ] ]';
const expected4 = 'Proxy [ Proxy [ {}, {} ], Proxy [ Proxy [ {}, {} ], {} ] ]';
const expected5 = 'Proxy [ Proxy [ Proxy [ Proxy [Array], {} ],' +
const expected5 = 'Proxy [ Proxy [ Proxy [ Proxy [ {}, {} ], {} ],' +
' Proxy [ {}, {} ] ],\n Proxy [ Proxy [ {}, {} ]' +
', Proxy [ Proxy [Array], {} ] ] ]';
const expected6 = 'Proxy [ Proxy [ Proxy [ Proxy [Array], Proxy [Array]' +
' ],\n Proxy [ Proxy [Array], Proxy [Array] ] ],\n' +
' Proxy [ Proxy [ Proxy [Array], Proxy [Array] ],\n' +
' Proxy [ Proxy [Array], Proxy [Array] ] ] ]';
', Proxy [ Proxy [ {}, {} ], {} ] ] ]';
const expected6 = 'Proxy [ Proxy [ Proxy [ Proxy [ Proxy [ {}, {} ], {} ], ' +
'Proxy [ {}, {} ] ],\n' +
' Proxy [ Proxy [ {}, {} ], ' +
'Proxy [ Proxy [ {}, {} ], {} ] ] ],\n' +
' Proxy [ Proxy [ Proxy [ Proxy [ {}, {} ], {} ], ' +
'Proxy [ {}, {} ] ],\n' +
' Proxy [ Proxy [ {}, {} ], ' +
'Proxy [ Proxy [ {}, {} ], {} ] ] ] ]';
assert.strictEqual(
util.inspect(proxy1, { showProxy: true, depth: null }),
expected1);
Expand Down
26 changes: 13 additions & 13 deletions test/parallel/test-util-inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ assert.strictEqual(util.inspect({ a: 1, b: 2 }), '{ a: 1, b: 2 }');
assert.strictEqual(util.inspect({ 'a': {} }), '{ a: {} }');
assert.strictEqual(util.inspect({ 'a': { 'b': 2 } }), '{ a: { b: 2 } }');
assert.strictEqual(util.inspect({ 'a': { 'b': { 'c': { 'd': 2 } } } }),
'{ a: { b: { c: [Object] } } }');
'{ a: { b: { c: { d: 2 } } } }');
assert.strictEqual(
util.inspect({ 'a': { 'b': { 'c': { 'd': 2 } } } }, false, null),
'{ a: { b: { c: { d: 2 } } } }');
Expand Down Expand Up @@ -106,7 +106,7 @@ assert.strictEqual(util.inspect((new JSStream())._externalStream),
assert.strictEqual(util.inspect({ a: regexp }, false, 0), '{ a: /regexp/ }');
}

assert(/Object/.test(
assert(!/Object/.test(
util.inspect({ a: { a: { a: { a: {} } } } }, undefined, undefined, true)
));
assert(!/Object/.test(
Expand Down Expand Up @@ -1012,15 +1012,15 @@ if (typeof Symbol !== 'undefined') {
// Empty and circular before depth
{
const arr = [[[[]]]];
assert.strictEqual(util.inspect(arr), '[ [ [ [] ] ] ]');
assert.strictEqual(util.inspect(arr, { depth: 2 }), '[ [ [ [] ] ] ]');
arr[0][0][0][0] = [];
assert.strictEqual(util.inspect(arr), '[ [ [ [Array] ] ] ]');
assert.strictEqual(util.inspect(arr, { depth: 2 }), '[ [ [ [Array] ] ] ]');
arr[0][0][0] = {};
assert.strictEqual(util.inspect(arr), '[ [ [ {} ] ] ]');
assert.strictEqual(util.inspect(arr, { depth: 2 }), '[ [ [ {} ] ] ]');
arr[0][0][0] = { a: 2 };
assert.strictEqual(util.inspect(arr), '[ [ [ [Object] ] ] ]');
assert.strictEqual(util.inspect(arr, { depth: 2 }), '[ [ [ [Object] ] ] ]');
arr[0][0][0] = arr;
assert.strictEqual(util.inspect(arr), '[ [ [ [Circular] ] ] ]');
assert.strictEqual(util.inspect(arr, { depth: 2 }), '[ [ [ [Circular] ] ] ]');
}

// Corner cases.
Expand Down Expand Up @@ -1117,22 +1117,22 @@ if (typeof Symbol !== 'undefined') {
assert(!/1 more item/.test(util.inspect(arr)));
util.inspect.defaultOptions.maxArrayLength = oldOptions.maxArrayLength;
assert(/1 more item/.test(util.inspect(arr)));
util.inspect.defaultOptions.depth = null;
assert(!/Object/.test(util.inspect(obj)));
util.inspect.defaultOptions.depth = oldOptions.depth;
util.inspect.defaultOptions.depth = 2;
assert(/Object/.test(util.inspect(obj)));
util.inspect.defaultOptions.depth = oldOptions.depth;
assert(!/Object/.test(util.inspect(obj)));
assert.strictEqual(
JSON.stringify(util.inspect.defaultOptions),
JSON.stringify(oldOptions)
);

// Set multiple options through object assignment
util.inspect.defaultOptions = { maxArrayLength: null, depth: null };
util.inspect.defaultOptions = { maxArrayLength: null, depth: 2 };
assert(!/1 more item/.test(util.inspect(arr)));
assert(!/Object/.test(util.inspect(obj)));
assert(/Object/.test(util.inspect(obj)));
util.inspect.defaultOptions = oldOptions;
assert(/1 more item/.test(util.inspect(arr)));
assert(/Object/.test(util.inspect(obj)));
assert(!/Object/.test(util.inspect(obj)));
assert.strictEqual(
JSON.stringify(util.inspect.defaultOptions),
JSON.stringify(oldOptions)
Expand Down