Change node_buffer to accept ArrayBuffer

[jemjam]

• Apply changes to src/node_buffer for ArrayBuffers

Checklist

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

Affected core subsystem(s)

src

[jemjam]

Hey! Can anyone help me make this commit message better? @addaleax requested I pull this out from my other pull request here (#16042).

@TimothyGu helped me with this part, and I'm afraid my understanding of the C++ side of things is somewhat limited, so happy to make any changes requested.

[addaleax]

@JemBijoux This is fine, but just so you know, what I had in mind is a different commit rather than a completely separate PR – if this is easier for you, cool. :)

I think something like buffer: support ArrayBuffer in native C++ API should be fine as a commit message?

[addaleax]

@TimothyGu Actually, sorry to be a bit of a killjoy, but it would be nice if we could also make Buffer methods accept ArrayBuffers (including as this) in this case…

[TimothyGu]

@JemBijoux Thanks for refactoring! It's in my opinion that the changes in src/util.h from #16042 should be moved here as well, and the commit message be changed to src: accept ArrayBuffer in node_buffer, thus making clear that this PR is only about the C++ side of things, not the buffer module or its methods.

@addaleax Does ^^ answer your question?

[jemjam]

@addaleax Hah! I'm so embarrassed. (I'm used to the squash and merge flow usually.) Well not that embarrassed, but I did totally mistake your request. I can close and roll this back into the other (but as an additional commit on that branch). Maybe it's confusing if I keep making too many changes though, so confirm with me if I should or not...

@TimothyGu :: Updated commit message and included that header file here now. I should probably remove it from the other pr now, but just wanna be sure I shouldn't just roll this all back into that. If i should, I'll use that commit message for it. 👍

Hah, can you tell I'm new to all this...

[addaleax]

@JemBijoux Doing it in a separate PR works just the same for me :)

[addaleax]

Although, tbh, I am having more doubts that this is really a good change … it seems like the kind of thing that might lock us into supporting weird APIs? There are things that you can’t do with ArrayBuffers but that might be expected by APIs that check for Buffer::HasInstance() (like accessing indices)…

[jemjam]

Fair. I leave it to someone smarter than me to debate (since I'm not really aware of the implications). Just let me know.

[addaleax]

@JemBijoux Yeah, one of the problems I’m having as well is that it’s hard to actually see all the implications here – there’s no harm in thinking out loud about it yourself…

[Qard]

Seems reasonable. It'd be good to see some tests to verify ArrayBuffers really work properly here though, if at all possible.

[Qard]

You shouldn't need the v8:: prefixes.

[TimothyGu]

@addaleax

There are things that you can’t do with ArrayBuffers but that might be expected by APIs that check for Buffer::HasInstance() (like accessing indices)…

Buffer::HasInstance() already supports other TypedArrays so there's no guarantee what the type of indexed properties would be.

As a compromise, would it be acceptable to you to make all other Buffer methods/macros work with ArrayBuffer except for HasInstance()?

[addaleax]

@TimothyGu Yes, I think that would be better. It still seems like this would be breaking with the design goals of the ArrayBuffer vs ArrayBufferView distinction…

[addaleax]

@TimothyGu Coming back to this… a few thoughts:

• I think this maybe should be considered semver-major in its current state, because addons rely on HasInstance() for typechecking?
• I think the main reason this feels weird to me is that it’s not compatible with my understanding of how the ArrayBuffer(View) APIs were designed. I think it was an intentional decision to not let users access ArrayBuffer data directly without creating an ArrayBufferView, and deviating from that on the native side seems like an odd decision?
• The only use case that can’t be covered by ABVs directly would be multi-gigabyte memory handling, right? I kind of feel like, at least in the zlib case, you’d be in trouble anyway if you attempted this… more so for the decompression side than the compression side, but still…

I’m sorry, I really don’t want this PR to end up stalling. I still think turning off the check in HasInstance() would make me feel more comfortable with landing it

[TimothyGu]

@addaleax 👍 to your argument, but free time has been a rarity these couple of days, so would appreciate it if you and @JemBijoux could get this PR into a landable shape.

[BridgeAR]

Ping @JemBijoux

[jemjam]

Hi! I apologize for this being long running. I sorta bit off more than I could chew.

I think I mentioned earlier, but I don't really know what I'm doing in C++. (The PR was started during a code and learn, and when I got into it initially I thought I'd submit a js change; @TimothyGu really held my hand getting into most of this.)

I'm very happy to work with someone to refactor/update this, but would it make more sense to just close this? Or if this is still a reasonable start, does anyone want to take over, or at the very least, take some time on a call to explain or pair code through it with me? Unfortunately, as it stands, I can only vaguely make sense of what is being requested for changes. Tackling this by myself could be a little messy.

Inspired to help out and participate, but also a little out of my depth on this. 🙃

[TimothyGu]

I'm going to close this in favor of @addaleax's suggestion in #16042 (comment). Even though implementing that suggestion would mean roundtripping between ArrayBuffer->Uint8Array->ArrayBuffer (in C++) I don't think performance will be our main concern at this moment.

Thanks @JemBijoux for opening this. We can continue this over at #16042.