util: add `maxStringLength` option to `inspect` function#32392

rosaxxny · 2020-03-20T16:32:41Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2020-03-20T16:35:09Z

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

BridgeAR

@rosaxny thank you very much for the PR! I am not certain in what way this is related to the referenced issue though, since that would still happen.
I also think we should not add this option in general without having a steong reason to do so. If someone wants to limit the steing size, that could be done after the stringification or by hooking in the unsupported stylize function. For those reasons I am -1 on this PR.

BridgeAR · 2020-03-20T20:29:00Z

It is IMO also semver-major due to limiting information that was present before.

addaleax · 2020-03-20T20:38:26Z

@nodejs/TSC PTAL

I am not certain in what way this is related to the referenced issue though, since that would still happen.

@BridgeAR The process does not crash anymore when inspecting large strings in the REPL by default since it does not attempt to print a similarly large string.

Ultimately, this is no different from the maxArrayLength option in its purpose and functionality; it trades off providing some information in exchange for the returned information being more concise (and not crashing the process in extreme cases).

BridgeAR · 2020-03-20T20:42:45Z

AFAIK it should already crash while checking for the string length.

I am also no fan of the max array length. There are other possibilities to limit the total string length. The maxArrayLength is often to restrictive with the current default value.

addaleax · 2020-03-20T20:45:16Z

AFAIK it should already crash while checking for the string length.

It does not.

I am also no fan of the max array length. There are other possibilities to limit the total string length. The maxArrayLength is often to restrictive with the current default value.

I was skeptical about maxArrayLength at first too, but in the end I do feel very much that that was a good idea to introduce. It does keep the inspection output from becoming too overwhelming.

Would you remove your objection if the default was set to Infinity here, at least for now?

mcollina

lgtm

mmarchini

LGTM, but if we want this as semver-minor the default should be Infinity. We can always have a follow up changing the default right after this lands.

cjihrig

LGTM. I have a slight preference to naming the option maxStringLength, but it's up to you.

tniessen · 2020-03-21T15:38:01Z

Thanks for the PR! No objection, just a few thoughts:

Set to 0 or negative to show no characters.

This seems to be a side effect of the implementation, and not a likely use case. (I realize that this is likely copied from the maxArrayLength description.) Personally, I don't think negative values should be allowed at all, and should throw a RangeError instead. We might want to allow that behavior because it is not strictly illogical and consistent with maxArrayLength, but I would personally prefer restricting it to unsigned integers, or Infinity.

I have a slight preference to naming the option maxStringLength

+1 for consistency with the rest of the API. At least I don't remember any APIs that use "str" instead of "string". There are lots of APIs that use "string", e.g., toString, toLocaleString, MAX_STRING_LENGTH, StringDecoder, isString, isStringObject, querystring.

'aaaa... 999996 more characters'

The meta string (999996 more characters) is currently part of the string literal, making it impossible to syntactically distinguish the above output from the result of calling util.inspect('aaaa... 999996 more characters'). Personally, I could see this to be confusing. Maybe the result should be 'aaaa'... 999996 more characters instead, to allow the distinction?

rosaxxny · 2020-03-27T15:21:33Z

@cjihrig @tniessen Thanks for the feedback. I've made some changes. Could you have another look?

nodejs-github-bot · 2020-03-27T15:30:39Z

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

cjihrig · 2020-03-27T15:42:27Z

My comment has been addressed, thanks.

nodejs-github-bot · 2020-03-29T19:41:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30192/ (:heavy_check_mark:)

addaleax · 2020-04-02T23:55:49Z

I’m putting this on the @nodejs/tsc agenda since there’s a standing objection from @BridgeAR who hasn’t engaged since that.

@rosaxny Fyi, this would need to be rebased before it can land so that the merge conflict displayed by the GitHub UI is resolved.

cjihrig · 2020-04-02T23:59:26Z

If the objector is unresponsive, another Collaborator may dismiss the objection.

From the collaborator guide... I don't think we strictly need to involve the TSC for that.

addaleax · 2020-04-03T00:06:27Z

@cjihrig Hm yeah … I don’t want to just ignore somebody’s objection, even if I personally fully disagree with it. Unless there’s a reason to land this soon (Is there? I think the v14.0.0 semver-major cut has already passed? /cc @BethGriggs), I think waiting until the meeting should make sense?

cjihrig · 2020-04-03T00:08:59Z

I'm pretty sure the semver-major cutoff has passed, but at least this can easily be semver-minor with the right default value 😄

Refs: nodejs#25478

rosaxxny · 2020-04-03T16:52:43Z

@mmarchini @cjihrig I changed the default value to Infinity as suggested.

nodejs-github-bot · 2020-04-03T19:51:09Z

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

tniessen · 2020-04-03T19:51:56Z

Thanks @rosaxny. The CI failures are unrelated.

nodejs-github-bot · 2020-04-05T19:12:47Z

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

nodejs-github-bot · 2020-04-05T20:42:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30475/ (:heavy_check_mark:)

BridgeAR · 2020-04-08T15:39:26Z

Sorry for the late reply. I did not have the time to look at Node.js things in the last couple days.

This does indeed fix #25478. I would still rather have a functionality that is more general purpose similar to the stylize function but this is probably more intuitive for most people who struggle with huge string length. I am therefore fine landing this and removed it from the TSC agenda.

addaleax · 2020-04-08T16:03:14Z

Landed in 944d862 🎉

Refs: #25478 PR-URL: #32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refs: nodejs#32392

Refs: #25478 PR-URL: #32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refs: #32392 PR-URL: #32744 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Refs: #25478 PR-URL: #32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable changes: New file system APIs: * Added a new function, `fs.readv` (with sync and promisified versions). This function takes an array of `ArrayBufferView` elements and will write the data it reads sequentially to the buffers (Sk Sajidul Kadir). #32356 * A new overload is available for `fs.readSync`, which allows to optionally pass any of the `offset`, `length` and `position` parameters. #32460 Other changes: * dns: * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4 mapped IPv6 addresses (murgatroid99). #32183 * http: * The default maximum HTTP header size was changed from 8KB to 16KB (rosaxny). #32520 * n-api: * Calls to `napi_call_threadsafe_function` from the main thread can now return the `napi_would_deadlock` status in certain circumstances (Gabriel Schulhof). #32689 * util: * Added a new `maxStrLength` option to `util.inspect`, to control the maximum length of printed strings. Its default value is `Infinity` (rosaxny). #32392 * worker: * Added support for passing a `transferList` along with `workerData` to the `Worker` constructor (Juan José Arboleda). #32278 New core collaborators: With this release, we welcome three new Node.js core collaborators: * himself65. #32734 * flarna (Gerhard Stoebich). #32620 * mildsunrise (Alba Mendez). #32525 PR-URL: #32813

Refs: nodejs#25478 PR-URL: nodejs#32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Refs: #25478 PR-URL: #32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Added to `util.inspect` in nodejs/node#32392, in node v12.17, v13.13, v14+

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Mar 20, 2020

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 20, 2020

addaleax approved these changes Mar 20, 2020
View reviewed changes

BridgeAR requested changes Mar 20, 2020
View reviewed changes

BridgeAR added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 20, 2020

mcollina approved these changes Mar 20, 2020
View reviewed changes

mmarchini approved these changes Mar 20, 2020
View reviewed changes

cjihrig approved these changes Mar 21, 2020
View reviewed changes

tniessen changed the title ~~util: add maxStrLength option to inspect function~~util: add maxStringLength option to inspect functionApr 1, 2020

tniessen approved these changes Apr 1, 2020
View reviewed changes

addaleax added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 2, 2020

util: add maxStrLength option to inspect function
9f90e7b
Refs: nodejs#25478

rosaxxny force-pushed the repl/edit-util-inspect-str-length branch from 63a4bb4 to 9f90e7bCompare April 3, 2020 16:41

fixup! util: add maxStrLength option to inspect function
4836f81

jasnell approved these changes Apr 3, 2020
View reviewed changes

mhdawson mentioned this pull request Apr 6, 2020
Node.js Technical Steering Committee (TSC) Meeting 2020-04-08 nodejs/TSC#846
Closed

BridgeAR approved these changes Apr 8, 2020
View reviewed changes

BridgeAR removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 8, 2020

addaleax closed this Apr 8, 2020

rosaxxny added a commit to rosaxxny/node that referenced this pull request Apr 9, 2020
util: change default value of maxStringLength to 10000
dcb0c8a
Refs: nodejs#32392

rosaxxny mentioned this pull request Apr 9, 2020
util: change default value of maxStringLength to 10000 #32744
Closed
4 tasks

cjihrig mentioned this pull request Apr 10, 2020
Add maxStringLength option for util.inspect #27042
Closed

targos mentioned this pull request Apr 13, 2020
v13.13.0 release proposal #32813
Merged

targos mentioned this pull request May 2, 2020
v12.17.0 release proposal #33197
Merged

ljharb added a commit to inspect-js/object-inspect that referenced this pull request May 26, 2020
[New] add maxStringLength option
b3995cb
Added to `util.inspect` in nodejs/node#32392, in node v12.17, v13.13, v14+

Uh oh!

util: add maxStringLength option to inspect function#32392

util: add maxStringLength option to inspect function #32392

Uh oh!

Conversation

rosaxxny commented Mar 20, 2020

Checklist

Uh oh!

nodejs-github-bot commented Mar 20, 2020

Uh oh!

BridgeAR left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 20, 2020

Uh oh!

addaleax commented Mar 20, 2020

Uh oh!

BridgeAR commented Mar 20, 2020

Uh oh!

addaleax commented Mar 20, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mmarchini left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Mar 21, 2020

Uh oh!

rosaxxny commented Mar 27, 2020

Uh oh!

nodejs-github-bot commented Mar 27, 2020

Uh oh!

cjihrig commented Mar 27, 2020

Uh oh!

nodejs-github-bot commented Mar 29, 2020• edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Apr 2, 2020

Uh oh!

cjihrig commented Apr 2, 2020

Uh oh!

addaleax commented Apr 3, 2020

Uh oh!

cjihrig commented Apr 3, 2020

Uh oh!

rosaxxny commented Apr 3, 2020

Uh oh!

nodejs-github-bot commented Apr 3, 2020

Uh oh!

tniessen commented Apr 3, 2020

Uh oh!

nodejs-github-bot commented Apr 5, 2020

Uh oh!

nodejs-github-bot commented Apr 5, 2020• edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Apr 8, 2020

Uh oh!

addaleax commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

util: add `maxStringLength` option to `inspect` function#32392

util: add `maxStringLength` option to `inspect` function #32392

BridgeAR left a comment •
edited
Loading

nodejs-github-bot commented Mar 29, 2020•
edited by addaleax
Loading

nodejs-github-bot commented Apr 5, 2020•
edited by addaleax
Loading