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

New principle: vend byte arrays as Uint8Arrays #480

Merged
merged 3 commits into from
Apr 3, 2024

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Mar 11, 2024

Fixes #463.


Preview | Diff

index.bs Outdated
@@ -1856,6 +1856,19 @@ as the [Promise integration](https://github.com/WebAssembly/js-promise-integrati
is still under development as of today.
</div>

<h3 id="uint8array">Vend byte arrays as Uint8Arrays</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good advice, but I'd like to see the reciprocal as well, which is that APIs should take ArrayBufferView as input, not any specific type.

Nit:

Suggested change
<h3 id="uint8array">Vend byte arrays as Uint8Arrays</h3>
<h3 id="uint8array">Output an array of bytes with Uint8Array</h3>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is good advice, but I'd like to see the reciprocal as well, which is that APIs should take ArrayBufferView as input, not any specific type.

At least one API (encodeInto) takes a Uint8Array specifically, and I don't know that I agree it would be better for it to take an ArrayBufferView. It seems like a confusion of types for a user to pass a Uint16Array there, for example. Maybe that only applies when taking an input as a write target rather than just a source.

Also, wouldn't BufferSource be more appropriate? It is more common than ArrayBufferView, I'm pretty sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake, BufferSource was the one I was looking for.

encodeInto has an in-out argument, so perhaps it is a poor example of an input argument.

I'm interested in whether you think that people should be permitted to supply data in a form that best suits them. it seems like ArrayBufferView endianness issues might cause you to prefer something less likely to tempt that sort of concern, but I see that things like Web Crypto use it happily (noting that I'm quite receptive to arguments of the form "Web Crypto is not an API you should model behaviour on").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Were I designing web APIs from scratch, I would take only the narrowest appropriate TypedArray type, and in the case of byte-taking APIs specifically I'd take Uint8Arrays and ArrayBuffers (and no other types), since those both unambiguously represent a sequence of bytes, whereas (e.g.) a Uint16Array does not unambiguously represent a sequence of bytes.

Even leaving aside the issue of endianness, one can store byte values in a Uint16Array, and passing such a thing to a byte-taking API is not going to do what the user presumably expected. Requiring the user to explicitly do either new Uint8Array(uint16ArrayInstance) or new Uint8Array(uint16ArrayInstance.buffer, uint16ArrayInstance.byteOffset, uint16ArrayInstance.byteLength) (depending on which of those two behaviors they actually wanted) would be better. More generally, I think we'd all be better off if web platform APIs were much stricter about their input types (and I'm trying to move JS, at least, in that direction).

But I'm not designing web APIs from scratch, and in the world we actually live in, BufferSource is pretty widely used. So I'm neutral on this. I don't really want to be the one to codify "accept BufferSource" as a principle, though.

Copy link
Member

Choose a reason for hiding this comment

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

I guess there is a question as to whether we want to tackle this incrementally or offer advice for a variety of things. Shared memory also comes to mind as something that might need some advice.

And yeah, I think accepting BufferSource is probably the way to go, unless there's a very compelling reason not to have parity with the vast majority of existing APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resizable buffers too, come to that.

It's a bit hard to offer advice on those right now because it seems like web platform hasn't entirely figured out whether APIs should take BufferSources backed by shared / resizable buffers. Personally I think almost all places should take them, but [AllowResizable] and [AllowShared] are still default-off right now (IIUC just because that was the safe option) and few (if any?) places have been updated to support them.

I suppose starting with a principle that shared/resizable buffers should be accepted could be a way to start moving in that direction. But I think it would be best to do that in a different issue/PR, rather than holding up this one until that's done.

Co-authored-by: Martin Thomson <mt@lowentropy.net>
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Copy link
Member

@torgo torgo left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New principle: APIs which vend byte buffers should return a Uint8Array
4 participants