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 Set and Map size to the inspect output #30225

Closed
wants to merge 2 commits into from

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Nov 2, 2019

This adds the set or map size while serializing those with inspect().
That aligns the output with the one from Chromium. To do so, I refactored some code that special handled iterators that had the prototype set to null. That is not required anymore. It was necessary to bind some properties to later on used functions to keep the performance profile in tact and the implementation similar to the way it used to work.
Due to the refactoring a few more edge cases will also from now on be handled properly (inspecting a set or map with a broken iterator symbol or the size property manipulated will still work).

This should be semver-minor, since we explicitly note that people should not rely on the inspect output programmatically.

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
@BridgeAR BridgeAR force-pushed the BridgeAR:add-set-map-size branch from 9863eb1 to 8d6cd92 Nov 2, 2019
lib/internal/util/inspect.js Outdated Show resolved Hide resolved
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 2, 2019

Would it be a bit more intuitive (and more consistent) to handle this like we do for e.g. ArrayBuffers, i.e. print size as a property (which is how you’d access it in JS)?

> a = new ArrayBuffer(4)
ArrayBuffer { [Uint8Contents]: <00 00 00 00>, byteLength: 4 }
> a = new Map([[1,2],[3,4]])
Map { 1 => 2, 3 => 4, size: 2 }
@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 2, 2019

I don't think it's immediately clear what the number on the map means. Can we just show 'size' as a normal property? if someone overrides it they probably would want our inspection to show the overridden one anyway.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Nov 2, 2019

I am fine to print that instead. This implementation is just not possible to manipulate. The other one could show something else (I actually added a test for that).

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Nov 2, 2019

Just as a note: I personally think the shorter notation is nicer to read (even though it definitely requires a short moment to understand the number when seeing this notation for the first time).

I updated the implementation nevertheless. PTAL.

Copy link
Member

addaleax left a comment

I find the brackets around [size] weird and would recommend to omit them, but either way this LGTM

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Nov 2, 2019

Screen Shot 2019-11-03 at 3 36 37 AM

This is how things look like in the Chrome DevTools BTW.

Do we really need to display the size when the number of entries are small enough so that they are all displayed?

@BridgeAR BridgeAR added blocked and removed blocked labels Nov 17, 2019
@BridgeAR BridgeAR dismissed addaleax’s stale review Nov 18, 2019

Outdated due to a changed implementation.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Nov 18, 2019

I went ahead and created a twitter pool for this: https://twitter.com/BridgeAR/status/1195687117080408067
I tried to keep the question as neutral as possible.

Seems like 72% of all 115 participants would like to have the size as part of the default output. 28% are happy with the output as it is.

62.5% percent (of the 72%) are in favor of Set(n) { ... } or Map(n) { ... } while 37.5% would favor output similar to a property (the latter number is the sum of the output visualized as own property and as computed/prototype property).

I therefore went back to my initial implementation. @addaleax I removed your LG due to that change. PTAL

@joyeecheung we could in theory only add the number in a couple of cases (depending on the size). Set and Map entries can in general look very different though and in some cases it might be straight forward to see how many entries they are while it's more difficult in other cases. Thus I think it's best to be consistent. Especially as it might also be confusing to some users to see different output depending on the size.

@nodejs/util PTAL

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@BridgeAR BridgeAR force-pushed the BridgeAR:add-set-map-size branch from a9b559d to 402f9be Nov 26, 2019
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Nov 26, 2019

Rebased due to conflicts @nodejs/util this could use some reviews.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Dec 3, 2019

@targos
targos approved these changes Dec 6, 2019
@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 6, 2019

I think this can be semver-patch. There are no additions to the public API.
Edit: could stay semver-minor if you think the change is substantial enough.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 6, 2019

I am fine with this being a patch as well. We treat most of our util changes as patches. I'll follow your recommendation while I am fine both ways.

@BridgeAR BridgeAR removed the semver-minor label Dec 6, 2019
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Dec 7, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27448/ (yellow build with a known Windows flake)

This removes the special handling to inspect iterable objects with
a null prototype. It is now handled together with the regular
prototype.
@BridgeAR BridgeAR force-pushed the BridgeAR:add-set-map-size branch from 9cb9f7f to 4869c44 Dec 13, 2019
@nodejs-github-bot

This comment was marked as outdated.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 13, 2019

Rebased due to conflicts.

This adds the size of a set and map to the output. This aligns the
output with the one from Chromium.
@BridgeAR BridgeAR force-pushed the BridgeAR:add-set-map-size branch from 4869c44 to 96fb0b9 Dec 13, 2019
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Dec 14, 2019

@Trott
Trott approved these changes Dec 14, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 14, 2019

BridgeAR added a commit that referenced this pull request Dec 15, 2019
This removes the special handling to inspect iterable objects with
a null prototype. It is now handled together with the regular
prototype.

PR-URL: #30225
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request Dec 15, 2019
This adds the size of a set and map to the output. This aligns the
output with the one from Chromium.

PR-URL: #30225
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 15, 2019

Landed in eeae598, ab59989 🎉

@BridgeAR BridgeAR closed this Dec 15, 2019
MylesBorins added a commit that referenced this pull request Dec 17, 2019
This removes the special handling to inspect iterable objects with
a null prototype. It is now handled together with the regular
prototype.

PR-URL: #30225
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
This adds the size of a set and map to the output. This aligns the
output with the one from Chromium.

PR-URL: #30225
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
MylesBorins added a commit that referenced this pull request Dec 18, 2019
Notable Changes:

* cli:
  * add --trace-exit cli option (legendecas)
    #30516
* http,https:
  * increase server headers timeout (Tim Costa)
    #30071
* readline:
  * update ansi-regex (Ruben Bridgewater)
    #30907
  * promote \_getCursorPos to public api (Jeremy Albright)
    #30687
* repl:
  * add completion preview (Ruben Bridgewater)
    #30907
* util:
  * add Set and map size to inspect output (Ruben Bridgewater)
    #30225
* wasi:
  * require CLI flag to require() wasi module (Colin Ihrig)
    #30963

PR-URL: #31010
MylesBorins added a commit that referenced this pull request Dec 18, 2019
Notable Changes:

* cli:
  * add --trace-exit cli option (legendecas)
    #30516
* http,https:
  * increase server headers timeout (Tim Costa)
    #30071
* readline:
  * update ansi-regex (Ruben Bridgewater)
    #30907
  * promote \_getCursorPos to public api (Jeremy Albright)
    #30687
* repl:
  * add completion preview (Ruben Bridgewater)
    #30907
* util:
  * add Set and map size to inspect output (Ruben Bridgewater)
    #30225
* wasi:
  * require CLI flag to require() wasi module (Colin Ihrig)
    #30963

PR-URL: #31010
@targos

This comment has been minimized.

Copy link
Member

targos commented Jan 14, 2020

This doesn't land cleanly on v12.x-staging and probably depends on previous PRs that were not backported yet.

@BridgeAR BridgeAR deleted the BridgeAR:add-set-map-size branch Jan 20, 2020
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 20, 2020
This removes the special handling to inspect iterable objects with
a null prototype. It is now handled together with the regular
prototype.

PR-URL: nodejs#30225
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 20, 2020
This adds the size of a set and map to the output. This aligns the
output with the one from Chromium.

PR-URL: nodejs#30225
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 22, 2020
This removes the special handling to inspect iterable objects with
a null prototype. It is now handled together with the regular
prototype.

PR-URL: nodejs#30225
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 22, 2020
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.