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: inspect ArrayBuffers TypedArray as well #25006

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
6 participants
@BridgeAR
Copy link
Member

BridgeAR commented Dec 13, 2018

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
@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 13, 2018

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

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 13, 2018

@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

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 13, 2018

An alternative representations I thought about was:

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

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

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 13, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 14, 2018

@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 15, 2018

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

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 16, 2018

@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

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 18, 2018

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

  1. 'ArrayBuffer { <Uint8BufferView 68 65 6c 6c 6f>, byteLength: 4 }'
  2. 'ArrayBuffer { <Uint8Array 68 65 6c 6c 6f>, byteLength: 4 }'
  3. 'ArrayBuffer { <ArrayBufferView (Uint8) 68 65 6c 6c 6f>, byteLength: 4 }'
  4. 'ArrayBuffer { [TypedArray]: <Uint8Array 68 65 6c 6c 6f>, byteLength: 4 }'
  5. 'ArrayBuffer { <ArrayBufferView 68 65 6c 6c 6f>, byteLength: 4 }'
  6. 'ArrayBuffer { [ArrayBufferView as base64]: 'aGVsbG8=', byteLength: 4 }'
  7. '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

This comment has been minimized.

Copy link
Member

addaleax commented Dec 18, 2018

@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

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 18, 2018

@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.

  1. 'ArrayBuffer { <Content 68 65 6c 6c 6f>, byteLength: 4 }'
  2. 'ArrayBuffer { <Uint8Content 68 65 6c 6c 6f>, byteLength: 4 }'
  3. 'ArrayBuffer { [Content]: <68 65 6c 6c 6f>, byteLength: 4 }'
  4. 'ArrayBuffer { [Content]: <Uint8 68 65 6c 6c 6f>, byteLength: 4 }'
  5. 'ArrayBuffer { [Uint8Content]: <68 65 6c 6c 6f>, byteLength: 4 }'
@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 18, 2018

Alternatives: Data, Contents

@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 19, 2018

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 19, 2018

@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

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 19, 2018

@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

This comment has been minimized.

Copy link
Member

Trott commented Dec 22, 2018

@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.

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

@BridgeAR BridgeAR force-pushed the BridgeAR:inspect-array-buffer-closer branch from a5100e0 to f58d87c Dec 24, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 24, 2018

PTAL. I reworked the output as discussed.

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

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 27, 2018

@nodejs/util @addaleax @targos PTAL

This needs some reviews.

util: inspect ArrayBuffers contents as well
Inspecting an ArrayBuffer now also shows their binary contents.

@BridgeAR BridgeAR force-pushed the BridgeAR:inspect-array-buffer-closer branch from f58d87c to 48bde7a Dec 27, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 27, 2018

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

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

@targos

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 7, 2019

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 9, 2019

Landed in aa07dd6

@addaleax addaleax closed this Jan 9, 2019

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

util: inspect ArrayBuffers contents as well
Inspecting an ArrayBuffer now also shows their binary contents.

PR-URL: #25006
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 9, 2019

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

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

util: inspect ArrayBuffers contents as well
Inspecting an ArrayBuffer now also shows their binary contents.

PR-URL: nodejs#25006
Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

util: inspect ArrayBuffers contents as well
Inspecting an ArrayBuffer now also shows their binary contents.

PR-URL: nodejs#25006
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@BridgeAR BridgeAR referenced this pull request Jan 9, 2019

Closed

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

4 of 4 tasks complete

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

util: inspect ArrayBuffers contents as well
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
Inspecting an ArrayBuffer now also shows their binary contents.

PR-URL: #25006
Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

util: inspect ArrayBuffers contents as well
Inspecting an ArrayBuffer now also shows their binary contents.

PR-URL: #25006
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019

Merged

v11.7.0 proposal #25537

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

2019-01-17, Version 11.7.0 (Current), @BridgeAR
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

PR-URL: nodejs#25537

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

2019-01-17, Version 11.7.0 (Current), @BridgeAR
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

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

2019-01-17, Version 11.7.0 (Current), @BridgeAR
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

@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

Merged

v11.8.0 proposal #25687

@targos targos added this to Backported in v11.x Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment