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.

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: support inspecting Map, Set, and Promise #1471

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
121 changes: 108 additions & 13 deletions lib/util.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

const uv = process.binding('uv');
const Debug = require('vm').runInDebugContext('Debug');
Copy link
Contributor

Choose a reason for hiding this comment

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

@domenic: Should we add support for Weak{Set,Map} inspection through Debug as a follow-on to this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Noooo that seems very bad, basically exposes iteration to JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah – I thought it might be a bit fraught with peril; was surprised to see the chrome dev tools expose that info.


const formatRegExp = /%[sdj%]/g;
exports.format = function(f) {
Expand Down Expand Up @@ -192,6 +193,14 @@ function arrayToHash(array) {
}


function inspectPromise(p) {
var mirror = Debug.MakeMirror(p, true);
if (!mirror.isPromise())
return null;
return {status: mirror.status(), value: mirror.promiseValue().value_};
}


function formatValue(ctx, value, recurseTimes) {
// Provide a hook for user-specified inspect functions.
// Check that value is an object with an inspect function on it
Expand Down Expand Up @@ -276,14 +285,44 @@ function formatValue(ctx, value, recurseTimes) {
}
}

var base = '', array = false, braces = ['{', '}'];
var base = '', empty, braces, formatter;
Copy link
Member

Choose a reason for hiding this comment

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

Preferably assign empty = false and use strict checks below (i.e. empty = empty === true && keys.length === 0 and if (empty === true) {.)

It's a minor thing but V8 emits marginally tighter code for strict boolean comparisons.


// Make Array say that they are Array
if (Array.isArray(value)) {
array = true;
braces = ['[', ']'];
empty = value.length === 0;
formatter = formatArray;
} else if (value instanceof Set) {
braces = ['Set {', '}'];
// With `showHidden`, `length` will display as a hidden property for
// arrays. For consistency's sake, do the same for `size`, even though this
// property isn't selected by Object.getOwnPropertyNames().
if (ctx.showHidden)
keys.unshift('size');
empty = value.size === 0;
formatter = formatSet;
} else if (value instanceof Map) {
braces = ['Map {', '}'];
// ditto
if (ctx.showHidden)
keys.unshift('size');
empty = value.size === 0;
formatter = formatMap;
} else {
// Only create a mirror if the object superficially looks like a Promise
var promiseInternals = value instanceof Promise && inspectPromise(value);
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, is there a reason to keep the instanceof check out of inspectPromise?

if (promiseInternals) {
braces = ['Promise {', '}'];
empty = false;
formatter = formatPromise;
} else {
braces = ['{', '}'];
empty = true; // no other data than keys
Copy link
Member

Choose a reason for hiding this comment

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

Femto-style nit: two spaces before the comment and please capitalize and punctuate it. Ditto for other comments.

formatter = formatObject;
}
}

empty = empty && keys.length === 0;

// Make functions say that they are functions
if (typeof value === 'function') {
var n = value.name ? ': ' + value.name : '';
Expand Down Expand Up @@ -323,7 +362,7 @@ function formatValue(ctx, value, recurseTimes) {
base = ' ' + '[Boolean: ' + formatted + ']';
}

if (keys.length === 0 && (!array || value.length === 0)) {
if (empty) {
return braces[0] + base + braces[1];
}

Expand All @@ -337,14 +376,7 @@ function formatValue(ctx, value, recurseTimes) {

ctx.seen.push(value);

var output;
if (array) {
output = formatArray(ctx, value, recurseTimes, visibleKeys, keys);
} else {
output = keys.map(function(key) {
return formatProperty(ctx, value, recurseTimes, visibleKeys, key, array);
});
}
var output = formatter(ctx, value, recurseTimes, visibleKeys, keys);

ctx.seen.pop();

Expand Down Expand Up @@ -397,6 +429,13 @@ function formatError(value) {
}


function formatObject(ctx, value, recurseTimes, visibleKeys, keys) {
return keys.map(function(key) {
return formatProperty(ctx, value, recurseTimes, visibleKeys, key, false);
});
}


function formatArray(ctx, value, recurseTimes, visibleKeys, keys) {
var output = [];
for (var i = 0, l = value.length; i < l; ++i) {
Expand All @@ -417,6 +456,59 @@ function formatArray(ctx, value, recurseTimes, visibleKeys, keys) {
}


function formatSet(ctx, value, recurseTimes, visibleKeys, keys) {
var output = [];
value.forEach(function(v) {
var str = formatValue(ctx, v,
recurseTimes === null ? null : recurseTimes - 1);
Copy link
Member

Choose a reason for hiding this comment

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

For readability, it's probably better to assign the result of the ternary to a variable here and below.

Aside: am I the only one who finds it moronic that recurseTimes is sometimes a number and sometimes not?

output.push(str);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we should be able to do something like for (var v of value) { here instead of creating a closure.

keys.forEach(function(key) {
output.push(formatProperty(ctx, value, recurseTimes, visibleKeys,
key, false));
});
return output;
}


function formatMap(ctx, value, recurseTimes, visibleKeys, keys) {
var output = [];
value.forEach(function(v, k) {
var str = formatValue(ctx, k,
recurseTimes === null ? null : recurseTimes - 1);
str += ' => ';
str += formatValue(ctx, v, recurseTimes === null ? null : recurseTimes - 1);
output.push(str);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly, for (var tuple of value) { var k = tuple[0], v = tuple[1] }

keys.forEach(function(key) {
output.push(formatProperty(ctx, value, recurseTimes, visibleKeys,
key, false));
});
return output;
}

function formatPromise(ctx, value, recurseTimes, visibleKeys, keys) {
var output = [];
var internals = inspectPromise(value);
if (internals.status === 'pending') {
output.push('<pending>');
} else {
var str = formatValue(ctx, internals.value,
recurseTimes === null ? null : recurseTimes - 1);
if (internals.status === 'rejected') {
output.push('<rejected> ' + str);
} else {
output.push(str);
}
}
keys.forEach(function(key) {
output.push(formatProperty(ctx, value, recurseTimes, visibleKeys,
key, false));
});
return output;
}


function formatProperty(ctx, value, recurseTimes, visibleKeys, key, array) {
var name, str, desc;
desc = Object.getOwnPropertyDescriptor(value, key) || { value: value[key] };
Expand Down Expand Up @@ -488,7 +580,10 @@ function reduceToSingleString(output, base, braces) {

if (length > 60) {
return braces[0] +
(base === '' ? '' : base + '\n ') +
// If the opening "brace" is too large, like in the case of "Set {",
// we need to force the first item to be on the next line or the
// items will not line up correctly.
(base === '' && braces[0].length === 1 ? '' : base + '\n ') +
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this case needs tests

' ' +
output.join(',\n ') +
' ' +
Expand Down
63 changes: 63 additions & 0 deletions test/parallel/test-util-inspect.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,66 @@ if (typeof Symbol !== 'undefined') {
assert.equal(util.inspect(subject, options), '[ 1, 2, 3, [length]: 3, [Symbol(symbol)]: 42 ]');

}

// test Set
assert.equal(util.inspect(new Set), 'Set {}');
assert.equal(util.inspect(new Set([1, 2, 3])), 'Set { 1, 2, 3 }');
var set = new Set(["foo"]);
Copy link
Member

Choose a reason for hiding this comment

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

Femto-nit: prefer single quotes.

set.bar = 42;
assert.equal(util.inspect(set, true), 'Set { \'foo\', [size]: 1, bar: 42 }');

// test Map
assert.equal(util.inspect(new Map), 'Map {}');
assert.equal(util.inspect(new Map([[1, 'a'], [2, 'b'], [3, 'c']])), 'Map { 1 => \'a\', 2 => \'b\', 3 => \'c\' }');
Copy link
Member

Choose a reason for hiding this comment

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

Style: long line, please wrap at 80 columns.

var map = new Map([["foo", null]]);
map.bar = 42;
assert.equal(util.inspect(map, true), 'Map { \'foo\' => null, [size]: 1, bar: 42 }');

// test Promise
assert.equal(util.inspect(Promise.resolve(3)), 'Promise { 3 }');
assert.equal(util.inspect(Promise.reject(3)), 'Promise { <rejected> 3 }');
assert.equal(util.inspect(new Promise(function() {})), 'Promise { <pending> }');
var promise = Promise.resolve('foo');
promise.bar = 42;
assert.equal(util.inspect(promise), 'Promise { \'foo\', bar: 42 }');

// Make sure it doesn't choke on polyfills. Unlike Set/Map, there is no standard
// interface to synchronously inspect a Promise, so our techniques only work on
// a bonafide native Promise.
var oldPromise = Promise;
global.Promise = function() { this.bar = 42; };
assert.equal(util.inspect(new Promise), '{ bar: 42 }');
global.Promise = oldPromise;


// Test alignment of items in container
// Assumes that the first numeric character is the start of an item.

function checkAlignment(container) {
var lines = util.inspect(container).split("\n");
var pos;
lines.forEach(function(line) {
var npos = line.search(/\d/);
if (npos !== -1) {
if (pos !== undefined)
assert.equal(pos, npos, "container items not aligned");
pos = npos;
}
});
}

var big_array = [];
for (var i = 0; i < 100; i++) {
big_array.push(i);
}

checkAlignment(big_array);
checkAlignment(function(){
Copy link
Member

Choose a reason for hiding this comment

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

Style: space before brace.

var obj = {};
big_array.forEach(function(v) {
obj[v] = null;
});
return obj;
}());
checkAlignment(new Set(big_array));
checkAlignment(new Map(big_array.map(function (y) { return [y, null] })));