ipfs / protons

Protocol Buffers for Node.js and the browser without eval
Other
32 stars 23 forks source link

fix: replace node buffers with uint8arrays #14

Closed achingbrain closed 4 years ago

achingbrain commented 4 years ago

All internal use of node Buffers have been replaced with browser-friendly Uint8Arrays and DataViews.

Also refactors test suite to run on browsers.

BREAKING CHANGE:

jacobheun commented 4 years ago

The switch to aegir is causing mac and windows builds to fail because it can't run the browser tests. Need to update the travis file to specifically test browsers.

achingbrain commented 4 years ago

I find it strange that decode / encode functions take both buffer (which I think is Uint8Array now) and dataView arguments. It appeals to me that it would be better to just have a dataView argument.

Yeah, it looks a bit weird - I may have missed something in the API, but I couldn't see how to get a Uint8Array out of a DataView without creating a new instance from the internal ArrayBuffer.

The structure of the module is that it uses the varint and signed-varint modules to do most of the heavy lifting which don't accept DataViews, only ArrayLikes. This is an incredibly hot codepath so I didn't want to be creating new objects for a single setXXX call (or alternatively create a new object just to pass to varint) then have it have to garbage collect the new object shortly afterwards.

Node's Buffer API combines DataView-style multiple number type getter/setters with the byte-array semantics of Uint8Arrays but the browser API doesn't but we need both, so we create a DataView at the start of an encode/decode and pass the Uint8Array and the DataView and each encode/decode function can choose which one to use.

It is also worth considering that dataView passed may not be a view for the same buffer, which code seems to assume but does not verify that is the case.

I guess you could pass a DataView instance to the top-level encode/decode invocation to do this, but you know, garbage in, garbage out.