util: inspect ArrayBuffers TypedArray as well by BridgeAR · Pull Request #25006 · nodejs/node

BridgeAR · 2018-12-13T04:29:10Z

Inspecting an ArrayBuffer alone now also shows the Uint8Array to which the ArrayBuffer belongs to.

I visualized it as special property even though it's not a property. I have no strong opinion about that but it seemed like the best representation.

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 · 2018-12-13T04:29:11Z

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1919/pipeline

targos · 2018-12-13T06:06:53Z

I find this very confusing. ArrayBuffers don't have an underlying Uint8Array, it's the opposite.

BridgeAR · 2018-12-13T06:12:00Z

@targos yes, I described it wrong. The point is, that inspecting two different ArrayBuffer directly can result in the same output while belonging to a very different TypedArray. This makes sure it's clear to the viewer.

I am also happy with a different representation but this was the best I could come up with.

BridgeAR · 2018-12-13T06:45:44Z

An alternative representations I thought about was:

ArrayBuffer { byteLength: n } [Uint8Array] [...]

doc/api/util.md

BridgeAR · 2018-12-13T14:03:56Z

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

BridgeAR · 2018-12-14T17:24:59Z

@nodejs/util PTAL

targos · 2018-12-15T11:01:10Z

Since ArrayBuffers are the data holders for all typed arrays, I don't think it's a good idea to show the contents as Uint8Array. How about we do the same as with Node's Buffers and display it in hexadecimal form?
I'm also fine with the current state of things. It's easy enough to wrap the ArrayBuffer in any typed array to have the desired output.

BridgeAR · 2018-12-16T22:44:07Z

@targos I am fine with using a hexadecimal representation but I am not sure how to name the "property" (description) in that case. I could keep it as Uint8Array but it might be somewhat confusing that way?

About the current state: I think it's nice to see as much as possible right away but more important: it helps identifying different ArrayBuffers from each other right away. This also has impact on e.g. comparing two ArrayBuffers with assert as the error message could be is misleading until it's possible to distinguish them.

BridgeAR · 2018-12-18T11:32:05Z

@targos @addaleax what do you think of either of these representations (or any potential combination of some of these):

'ArrayBuffer { <Uint8BufferView 68 65 6c 6c 6f>, byteLength: 4 }'
'ArrayBuffer { <Uint8Array 68 65 6c 6c 6f>, byteLength: 4 }'
'ArrayBuffer { <ArrayBufferView (Uint8) 68 65 6c 6c 6f>, byteLength: 4 }'
'ArrayBuffer { [TypedArray]: <Uint8Array 68 65 6c 6c 6f>, byteLength: 4 }'
'ArrayBuffer { <ArrayBufferView 68 65 6c 6c 6f>, byteLength: 4 }'
'ArrayBuffer { [ArrayBufferView as base64]: 'aGVsbG8=', byteLength: 4 }'
'ArrayBuffer { <Base64 ArrayBufferView aGVsbG8>, byteLength: 4 }'

I personally think it's important to tell the user that it is represented as Uint8. In what way and with what other description is not really as important for me. Without that information it's hard to understand what the representation actually stands for.

addaleax · 2018-12-18T11:51:09Z

@BridgeAR I think 2, 3, 4, 5, 6 are misleading because they imply an associated ArrayBufferView in one way or another that does not actually exist. I think 6 and 7 are not useful, because base64 does not provide much visual information about the contents, only a way to reconstruct it.

I’d prefer for visual clutter/“specialness” to be minimal, so maybe a pseudo-property like [Uint8Contents]: <68 65 6c 6c 6f> would work?

I personally think it's important to tell the user that it is represented as Uint8.

I’m not sure I agree – we don’t do this for Buffers either, hex characters grouped in 2s are pretty much the standard representation for uint8 sequences, so it’s not really ambiguous.

BridgeAR · 2018-12-18T14:18:20Z

@addaleax

hex characters grouped in 2s are pretty much the standard representation for uint8 sequences, so it’s not really ambiguous

Sounds fine to me.

I’d prefer for visual clutter/“specialness” to be minimal

Coming from your suggestion, here's a new set of possibilities. I personally prefer 1 and 2 while not having a strong preference for either. I think 3 and 5 lack a description of the "value" (looking at it alone), otherwise I would also go for a pseudo property.

'ArrayBuffer { <Content 68 65 6c 6c 6f>, byteLength: 4 }'
'ArrayBuffer { <Uint8Content 68 65 6c 6c 6f>, byteLength: 4 }'
'ArrayBuffer { [Content]: <68 65 6c 6c 6f>, byteLength: 4 }'
'ArrayBuffer { [Content]: <Uint8 68 65 6c 6c 6f>, byteLength: 4 }'
'ArrayBuffer { [Uint8Content]: <68 65 6c 6c 6f>, byteLength: 4 }'

targos · 2018-12-18T14:22:30Z

Alternatives: Data, Contents

targos · 2018-12-19T12:36:17Z

/cc @nodejs/util

addaleax · 2018-12-19T14:11:16Z

@BridgeAR I think my personal preferences would be 5, 3, 2, 1, but let’s see what others think :)

Re: content vs contents, maybe it’s best to leave that to a native speaker?

BridgeAR · 2018-12-19T15:20:38Z

@addaleax the difference between content and contents is AFAIK subtle and depends on if we see the contained information as a whole (content) or if we consider it to be separate parts of information (contents). But by all means: I should not be the person to decide this :D (and I am also fine to use Data).

@Trott @vsemozhetbyt would you two be so kind and have a look at this? :)

Thinking about it: we could maybe use a team (e.g. nodejs/native-english-speaker) for pinging people with a very good English language knowledge (ideally both, British and American English. I for example struggle that)?

@targos do you have a preference for the last suggestions?

Trott · 2018-12-22T23:10:19Z

@Trott @vsemozhetbyt would you two be so kind and have a look at this? :)

In this particular case, I think contents is slightly more appropriate than content, but either will do. https://dictionary.cambridge.org/us/grammar/british-grammar/content-or-contents is a pretty good explanation of the subtlety. content is something that is uncountable, whereas contents is something that is enumerable. So in this case, contents is better, but no one will think it sounds strange if you use content instead.

doc/api/util.md

BridgeAR · 2018-12-24T00:06:52Z

PTAL. I reworked the output as discussed.

CI https://ci.nodejs.org/job/node-test-pull-request/19772/ ✔️

BridgeAR · 2018-12-27T15:20:45Z

@nodejs/util @addaleax @targos PTAL

This needs some reviews.

Inspecting an ArrayBuffer now also shows their binary contents.

BridgeAR · 2018-12-27T21:52:21Z

Rebased due to conflicts. PTAL, this needs a review.

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

targos · 2019-01-01T15:10:55Z

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

addaleax · 2019-01-07T22:25:01Z

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

addaleax · 2019-01-09T00:35:16Z

Landed in aa07dd6

Inspecting an ArrayBuffer now also shows their binary contents. PR-URL: #25006 Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax · 2019-01-09T00:41:22Z

@BridgeAR fyi, this would need a manual backport to v11.x-staging

Inspecting an ArrayBuffer now also shows their binary contents. PR-URL: nodejs#25006 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Inspecting an ArrayBuffer now also shows their binary contents. PR-URL: #25006 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable Changes * compression / zlib: * Added brotli support (Anna Henningsen and Zach Vacura) nodejs#24938 * console: * Added `inspectOptions` option (Ruben Bridgewater) nodejs#24978 * crypto: * Always accept private keys as public keys (Tobias Nießen) nodejs#25217 * deps: * Upgrade npm to v6.5.0 (Jordan Harband) nodejs#25234 * fs: * Use internalBinding('fs') internally instead of process.binding('fs') (Masashi Hirano) nodejs#22478 * http(s): * Support overriding http\\s.globalAgent (Roy Sommer) nodejs#25170 * util: * Inspect ArrayBuffers contents closely (Ruben Bridgewater) nodejs#25006 * worker: * Expose workers by default and remove `--experimental-worker` flag (Anna Henningsen) nodejs#25361 PR-URL: nodejs#25537

Notable Changes * compression / zlib: * Added brotli support (Anna Henningsen and Zach Vacura) #24938 * console: * Added `inspectOptions` option (Ruben Bridgewater) #24978 * crypto: * Always accept private keys as public keys (Tobias Nießen) #25217 * deps: * Upgrade npm to v6.5.0 (Jordan Harband) #25234 * fs: * Use internalBinding('fs') internally instead of process.binding('fs') (Masashi Hirano) #22478 * http(s): * Support overriding http\\s.globalAgent (Roy Sommer) #25170 * util: * Inspect ArrayBuffers contents closely (Ruben Bridgewater) #25006 * worker: * Expose workers by default and remove `--experimental-worker` flag (Anna Henningsen) #25361 PR-URL: #25537

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Dec 13, 2018

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 13, 2018

addaleax reviewed Dec 13, 2018

View reviewed changes

doc/api/util.md Outdated Show resolved Hide resolved

vsemozhetbyt reviewed Dec 23, 2018

View reviewed changes

doc/api/util.md Outdated Show resolved Hide resolved

BridgeAR force-pushed the inspect-array-buffer-closer branch from a5100e0 to f58d87c Compare December 24, 2018 00:04

util: inspect ArrayBuffers contents as well

48bde7a

Inspecting an ArrayBuffer now also shows their binary contents.

BridgeAR force-pushed the inspect-array-buffer-closer branch from f58d87c to 48bde7a Compare December 27, 2018 21:50

fixup: lazy load because of circular dependency

bd634cb

addaleax approved these changes Dec 30, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 7, 2019

addaleax closed this Jan 9, 2019

addaleax pushed a commit that referenced this pull request Jan 9, 2019

util: inspect ArrayBuffers contents as well

aa07dd6

Inspecting an ArrayBuffer now also shows their binary contents. PR-URL: #25006 Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax added the backport-requested-v11.x label Jan 9, 2019

BridgeAR mentioned this pull request Jan 9, 2019

[v11.x] backport #25087, #24962, #24931 and #25006 #25405

Closed

4 tasks

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 9, 2019

util: inspect ArrayBuffers contents as well

7472860

Inspecting an ArrayBuffer now also shows their binary contents. PR-URL: nodejs#25006 Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR added a commit that referenced this pull request Jan 10, 2019

util: inspect ArrayBuffers contents as well

dc4e5c5

Inspecting an ArrayBuffer now also shows their binary contents. PR-URL: #25006 Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR added backported-to-v11.x and removed backport-requested-v11.x labels Jan 10, 2019

addaleax pushed a commit that referenced this pull request Jan 14, 2019

util: inspect ArrayBuffers contents as well

901d3d0

Inspecting an ArrayBuffer now also shows their binary contents. PR-URL: #25006 Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR mentioned this pull request Jan 16, 2019

v11.7.0 proposal #25537

Merged

MylesBorins mentioned this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

BethGriggs added the backport-requested-v10.x label Aug 6, 2019

BridgeAR deleted the inspect-array-buffer-closer branch January 20, 2020 11:51

Uh oh!

Conversation

BridgeAR commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Dec 13, 2018

Uh oh!

targos commented Dec 13, 2018

Uh oh!

BridgeAR commented Dec 13, 2018

Uh oh!

BridgeAR commented Dec 13, 2018

Uh oh!

Uh oh!

BridgeAR commented Dec 13, 2018

Uh oh!

BridgeAR commented Dec 14, 2018

Uh oh!

targos commented Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Dec 16, 2018

Uh oh!

BridgeAR commented Dec 18, 2018

Uh oh!

addaleax commented Dec 18, 2018

Uh oh!

BridgeAR commented Dec 18, 2018

Uh oh!

targos commented Dec 18, 2018

Uh oh!

targos commented Dec 19, 2018

Uh oh!

addaleax commented Dec 19, 2018

Uh oh!

BridgeAR commented Dec 19, 2018

Uh oh!

Trott commented Dec 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

BridgeAR commented Dec 24, 2018 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Dec 27, 2018

Uh oh!

BridgeAR commented Dec 27, 2018

Uh oh!

targos commented Jan 1, 2019

Uh oh!

addaleax commented Jan 7, 2019

Uh oh!

addaleax commented Jan 9, 2019

Uh oh!

addaleax commented Jan 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

BridgeAR commented Dec 13, 2018 •

edited

Loading

targos commented Dec 15, 2018 •

edited

Loading

Trott commented Dec 22, 2018 •

edited

Loading

BridgeAR commented Dec 24, 2018 •

edited by Trott

Loading