util: truncate inspect array and typed array by jasnell · Pull Request #6334 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

util

Description of change

As an alternative to #5070, set the max length of Arrays/TypedArrays in util.inspect() to 100 and provide a maxArrayLength option to override.

Refs: #4905

Refs: #3648

tiny nits: Maybe capitalize Array elements consistently in this line, and enclose the constant 0 in backticks?

Maybe copy the added tests to cover TypedArray, too? Either way, LGTM.

@addaleax ... done! ;-)

What's so special about arrays? Why not make it a more general maxBytes that would apply to objects and strings too? -1 from me in the current form.

Oh, and the option for infinity is inconsistent with the depth option of the same function, where null means unrestricted logging.

We already have depth for objects. Would not be opposed to adding a limit for strings. maxBytes could be a bit difficult to track based on how inspect is building things out. Not impossible, of course, just a bit more complicated. Perhaps we can make the option maxLength and have it mean maximum number of characters when inspecting a string, and mean maximum number of elements when inspecting an array?

oh, and to answer the question about what's special about arrays ... attempting to inspect an array that's too large brings the node process down. I have no issue with evolving this to place other limits, but limiting the array output is the bit we currently have open issues on. See #5070

Updated, PTAL

Still LGTM.

I’d say arrays are also special because they are the appropriate data type for (possibly large amounts) of homogeneous data, so omitting out a number of items is probably usually okay. Maybe something like this change would make sense for strings too, but not for objects, because there would be no natural way of guessing which keys to leave out when printing.

@silverwind ... any further thoughts on this? do you still object?

@jasnell no, seems reasonable. I'm only concerned about the option. depth takes null, maxArrayLength takes null or Infinity. How about making it just null for consistency?

@silverwind ... I can do that... but how about only in docs? If someone happens to pass Infinity it would still just work.

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/2443/

@silverwind ... updated the doc to only mention setting maxArrayLength to null to specify no limit. Infinity can still be passed and it would work, but the only documented option is null to keep it consistent. Also made one other minor cleanup tweak. Running CI now. Assuming CI is green and there are no objections, I'll land on Monday.

CI is green.

Why not just set to Infinity for all elements?

See prior discussion.

lgtm if nits are addressed

If this is already major, maybe prefer moving the preference to Infinity over null for the other option? Or would it be better to do that in a new PR?

Infinity also work on depth, so I guess it's fine to leave it undocumented:

> util.inspect({a:{a:{a:{a:1}}}})
'{ a: { a: { a: [Object] } } }'
> util.inspect({a:{a:{a:{a:1}}}}, {depth: null})
'{ a: { a: { a: { a: 1 } } } }'
> util.inspect({a:{a:{a:{a:1}}}}, {depth: Infinity})
'{ a: { a: { a: { a: 1 } } } }'

LGTM

@silverwind @addaleax @mcollina ... any thoughts on whether this has to be semver-major or do you think we can treat it as a bug fix?

I don't think very many changes to util.inspect() should be considered major. It's primary purpose is creating human-readable output.

I guess semver-minor would be fine.

+1 to semver-minor.

New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/2484/
will land after if it's green.

As an alternative to nodejs#5070, set the max length of Arrays/TypedArrays in util.inspect() to `100` and provide a `maxArrayLength` option to override.

Green. unrelated flaky timeout in rpi.

As an alternative to #5070, set the max length of Arrays/TypedArrays in util.inspect() to `100` and provide a `maxArrayLength` option to override. PR-URL: #6334 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Landed in a2e5719

Ok for semver-minor.

Thank you @mcollina :-)

As an alternative to #5070, set the max length of Arrays/TypedArrays in util.inspect() to `100` and provide a `maxArrayLength` option to override. PR-URL: #6334 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

As an alternative to nodejs#5070, set the max length of Arrays/TypedArrays in util.inspect() to `100` and provide a `maxArrayLength` option to override. PR-URL: nodejs#6334 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

* assert: `deep{Strict}Equal()` now works correctly with circular references. (Rich Trott) #6432 * debugger: Arrays are now formatted correctly in the debugger repl. (cjihrig) #6448 * deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu) #6550 * net: Introduced a `Socket#connecting` property. (Fedor Indutny) #6404 - Previously this information was only available as the undocumented, internal `_connecting` property. * process: Introduced `process.cpuUsage()`. (Patrick Mueller) #6157 * stream: `Writable#setDefaultEncoding()` now returns `this`. (Alexander Makarenko) #5040 * util: Two new additions to `util.inspect()`: - Added a `maxArrayLength` option to truncate the formatting of Arrays. (James M Snell) #6334 - This is set to `100` by default. - Added a `showProxy` option for formatting proxy intercepting handlers. (James M Snell) #6465 - Inspecting proxies is non-trivial and as such this is off by default. PR-URL: #6557

* assert: `deep{Strict}Equal()` now works correctly with circular references. (Rich Trott) #6432 * debugger: Arrays are now formatted correctly in the debugger repl. (cjihrig) #6448 * deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu) #6550 - Please see our blog post for more info on the security contents of this release: - https://nodejs.org/en/blog/vulnerability/openssl-may-2016/ * net: Introduced a `Socket#connecting` property. (Fedor Indutny) #6404 - Previously this information was only available as the undocumented, internal `_connecting` property. * process: Introduced `process.cpuUsage()`. (Patrick Mueller) #6157 * stream: `Writable#setDefaultEncoding()` now returns `this`. (Alexander Makarenko) #5040 * util: Two new additions to `util.inspect()`: - Added a `maxArrayLength` option to truncate the formatting of Arrays. (James M Snell) #6334 - This is set to `100` by default. - Added a `showProxy` option for formatting proxy intercepting handlers. (James M Snell) #6465 - Inspecting proxies is non-trivial and as such this is off by default. PR-URL: #6557

jasnell added util Issues and PRs related to the built-in util module. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 21, 2016

jasnell mentioned this pull request Apr 21, 2016

util: compact inspect() for sparse arrays #5070

Closed

addaleax reviewed Apr 22, 2016
View reviewed changes

jasnell force-pushed the util-inspect-array branch from 5b8f4bb to 12c33ae Compare April 22, 2016 18:09

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

jasnell force-pushed the util-inspect-array branch from 12c33ae to 67acb26 Compare April 29, 2016 06:01

jasnell force-pushed the util-inspect-array branch from 67acb26 to 73524c6 Compare April 30, 2016 20:49

Fishrock123 reviewed May 2, 2016
View reviewed changes

jasnell added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 2, 2016

jasnell force-pushed the util-inspect-array branch from 73524c6 to 41cc07e Compare May 3, 2016 17:59

util: truncate inspect array and typed array

2b87241

As an alternative to nodejs#5070, set the max length of Arrays/TypedArrays in util.inspect() to `100` and provide a `maxArrayLength` option to override.

jasnell force-pushed the util-inspect-array branch from 41cc07e to 2b87241 Compare May 3, 2016 18:36

jasnell closed this May 3, 2016

This was referenced May 3, 2016

Node crashes when a big sparse array is given to console.log (might cause DoS) #4905

Closed

util.inspect is not so hot on large typed arrays #3648

Closed

This was referenced May 4, 2016

Propose v6.1.0 wrong base please ignore #6556

Closed

Propose v6.1.0 (Openssl update) #6557

Merged

BridgeAR mentioned this pull request Aug 25, 2018

util: improve util inspect performance #22503

Closed

4 tasks

lydell mentioned this pull request May 1, 2022

Truncate long Set and Map in util.inspect/console.log/REPL, just like long Array #42933

Closed

util: truncate inspect array and typed array by jasnell · Pull Request #6334 · nodejs/node · GitHub

Uh oh!

Uh oh!

util: truncate inspect array and typed array - #6334

util: truncate inspect array and typed array#6334jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:util-inspect-arrayjasnell/node:util-inspect-arrayCopy head branch name to clipboard

Conversation

jasnell commented Apr 21, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented Apr 22, 2016

Uh oh!

jasnell commented Apr 22, 2016

Uh oh!

addaleax Apr 22, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 22, 2016

Uh oh!

jasnell commented Apr 22, 2016

Uh oh!

silverwind commented Apr 22, 2016

Uh oh!

silverwind commented Apr 22, 2016

Uh oh!

jasnell commented Apr 22, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

addaleax commented Apr 29, 2016

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

silverwind commented Apr 30, 2016

Uh oh!

jasnell commented Apr 30, 2016

Uh oh!

mcollina commented Apr 30, 2016

Uh oh!

jasnell commented Apr 30, 2016

Uh oh!

jasnell commented Apr 30, 2016

Uh oh!

jasnell commented May 1, 2016

Uh oh!

Fishrock123 May 2, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell May 2, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented May 2, 2016

Uh oh!

Fishrock123 commented May 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016

Uh oh!

Fishrock123 commented May 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented May 2, 2016

Uh oh!

addaleax commented May 2, 2016

Uh oh!

jasnell commented May 3, 2016

Uh oh!

jasnell commented May 3, 2016

Uh oh!

jasnell commented May 3, 2016

Uh oh!

mcollina commented May 3, 2016

Uh oh!

jasnell commented May 3, 2016

Uh oh!

util: truncate inspect array and typed array#6334
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:util-inspect-array

Fishrock123 commented May 2, 2016 •
edited
Loading

Fishrock123 commented May 2, 2016 •
edited
Loading