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

Guarantee that BufferReader#buf is a Buffer #668

Closed
seishun opened this Issue Jan 30, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@seishun
Contributor

seishun commented Jan 30, 2017

I'm in the process of updating my encoder/decoder generator to match the API of protobuf.js 6.x (currently it mimics protobuf.js 4.x, you can see it here). The generated decode methods will take an object with properties buf and pos. buf will have to be a Buffer since the decoder will take advantage of Buffer's methods.

I also want to be able to pass the same data type to both my own decode and protobuf.js's decode. The latter accepts BufferReader, but the documentation for BufferReader says its buf is a Uint8Array, not Buffer, so it would be technically incompatible with my decode. But it's clear from the source code that it's actually a Buffer.

Can you update the documentation to state that BufferReader#buf is a Buffer and not just Uint8Array?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 30, 2017

The Uint8Array reference is used to simplify TypeScript usage (no need to override every single reader/writer function referencing Uint8Array that is inherited from the Uint8Array reader/writer), which is based on modern node version's Buffer inheriting from Uint8Array anyway.

Would it be enough if documentation would cover this textually, without changing the actual type to Buffer?

@seishun

This comment has been minimized.

Contributor

seishun commented Jan 30, 2017

I'm not sure how that would look like, but sure, as long as the documentation makes it clear that it's a Buffer I'd be okay with that.

Could you clarify why overriding all the functions would be necessary though?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 30, 2017

Could you clarify why overriding all the functions would be necessary though?

Sorry, not all functions, just the ones referencing Uint8Array. Because documentation.

@seishun

This comment has been minimized.

Contributor

seishun commented Jan 30, 2017

So just bytes()?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 30, 2017

That and #buf for BufferReader.

.alloc, #bytes, #finish and #buf for BufferWriter - and maybe all the Op stuff.

(thought it would be more)

Currently looking into this.

dcodeIO added a commit that referenced this issue Jan 30, 2017

@seishun

This comment has been minimized.

Contributor

seishun commented Jan 30, 2017

Seems reasonable to me, although I'm not sure what you mean by Op stuff.

I think it would be particularly useful to note that BufferWriter#finish returns a Buffer since that seems to be its primary difference from Writer.

Also BufferWriter#bytes doesn't need any doc change since it does actually accept Uint8Array, not only Buffer. And BufferWriter#buf doesn't exist.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 30, 2017

Updated, let me know if that solves your issue!

@dcodeIO dcodeIO added the enhancement label Jan 30, 2017

@seishun

This comment has been minimized.

Contributor

seishun commented Jan 30, 2017

Yes, thanks for the quick response!

@seishun seishun closed this Jan 30, 2017

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