-
Notifications
You must be signed in to change notification settings - Fork 49
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
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aUint16Array
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.
There was a problem hiding this comment.
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").There was a problem hiding this comment.
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)
ornew 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.