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: add maxStringLength option to inspect function #32392

Closed

Conversation

rosaxxny
Copy link
Contributor

Refs: #25478

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 nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Mar 20, 2020
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 20, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

@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 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
@BridgeAR
Copy link
Member

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

@addaleax
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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

@tniessen
Copy link
Member

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
Copy link
Contributor Author

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

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor

cjihrig commented Mar 27, 2020

My comment has been addressed, thanks.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 29, 2020

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

@tniessen tniessen changed the title util: add maxStrLength option to inspect function util: add maxStringLength option to inspect function Apr 1, 2020
@addaleax addaleax added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 2, 2020
@addaleax
Copy link
Member

addaleax commented Apr 2, 2020

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
Copy link
Contributor

cjihrig commented Apr 2, 2020

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
Copy link
Member

addaleax commented Apr 3, 2020

@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
Copy link
Contributor

cjihrig commented Apr 3, 2020

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

@rosaxxny rosaxxny force-pushed the repl/edit-util-inspect-str-length branch from 63a4bb4 to 9f90e7b Compare April 3, 2020 16:41
@rosaxxny
Copy link
Contributor Author

rosaxxny commented Apr 3, 2020

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

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member

tniessen commented Apr 3, 2020

Thanks @rosaxny. The CI failures are unrelated.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 5, 2020

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

@BridgeAR
Copy link
Member

BridgeAR commented Apr 8, 2020

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.

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

addaleax commented Apr 8, 2020

Landed in 944d862 🎉

addaleax pushed a commit that referenced this pull request Apr 8, 2020
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]>
@addaleax addaleax closed this Apr 8, 2020
rosaxxny added a commit to rosaxxny/node that referenced this pull request Apr 9, 2020
targos pushed a commit that referenced this pull request Apr 12, 2020
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]>
addaleax pushed a commit that referenced this pull request Apr 13, 2020
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]>
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
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]>
targos added a commit that referenced this pull request Apr 14, 2020
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
targos added a commit that referenced this pull request Apr 14, 2020
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
targos added a commit that referenced this pull request Apr 14, 2020
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
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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]>
targos pushed a commit that referenced this pull request Apr 28, 2020
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]>
ljharb added a commit to inspect-js/object-inspect that referenced this pull request May 26, 2020
Added to `util.inspect` in nodejs/node#32392, in node v12.17, v13.13, v14+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants