fix: remove node buffer #43
Conversation
Swaps node Buffer for Uint8Array in keys and values. BREAKING CHANGE: node Buffers have been replaced with Uint8Arrays
Updates to the latest interface-datastore that swaps out Buffers for Uint8Arrays. Depends on: - [ ] ipfs/interface-datastore#43 BREAKING CHANGE: no longer uses node Buffers, only Uint8Arrays
Updates to the latest changes from interface-datastore and datastore-core Depends on: - [ ] ipfs/interface-datastore#43 - [ ] ipfs/js-datastore-core#27 BREAKING CHANGES: only uses Uint8Arrays internally
This module didn't have a lot of buffer use in the first place, but we remove any references to buffers in favour of Uint8Arrays. When quering, values come back as Buffers which are Uint8Arrays so the conversion was unecessary. Depends on: - [ ] ipfs/interface-datastore#43 - [ ] ipfs/js-datastore-core#27 BREAKING CHANGE: remove node buffers in favour of Uint8Arrays
There was a problem hiding this comment.
@achingbrain I think comment flagged with 🚨 is worth addressing, others are just aside notes.
| * | ||
| * @returns {Buffer} | ||
| * @returns {Uint8Array} | ||
| */ | ||
| toBuffer () { | ||
| return this._buf |
There was a problem hiding this comment.
📝 I think this name is misleading now, maybe it should be renamed to toBytes or to toUint8Array ?
There was a problem hiding this comment.
A good suggestion, I've changed the method name to Key.uint8Array(). I dropped the to because we're not converting it to anything so it seems inaccurate.
I want to like the idea of .arrayBuffer() for symmetry/sympathy with other browser APIs but I'm struggling to see the utility of returning the underlying ArrayBuffer as the view is what we're interested in and there's a whole bunch of weirdness around what to do when the byteLengths are different.
I guess we can add it if we find that we need it.
src/key.js
Outdated
| } | ||
|
|
||
| if (this._buf[0] !== pathSep) { | ||
| this._buf = Buffer.concat([pathSepB, this._buf]) | ||
| this._buf = Uint8Array.of(pathSep, ...this._buf) |
There was a problem hiding this comment.
📝 I do not know how large ...this._buf can get, but JS engines (at least sure spidermonkey) have a cap on number of arguments function can take. If it can get arbitrary long it would be better to do
Uint8Array.from([pathSep, ...this._buf]) instead or this if this is on hot path:
const bytes = new Uint8Array(this._buf.byteLength + 1)
bytes.fill(pathSep, 0, 1)
bytes.set(this._buf, 1)
this._buf = bytesThere was a problem hiding this comment.
The second version is probably better as you avoid creating the intermediate array.
src/key.js
Outdated
| while (this._buf.length > 1 && this._buf[this._buf.length - 1] === pathSep) { | ||
| this._buf = this._buf.slice(0, -1) | ||
| while (this._buf.byteLength > 1 && this._buf[this._buf.byteLength - 1] === pathSep) { | ||
| this._buf = Uint8Array.of(...this._buf.slice(0, -1)) |
There was a problem hiding this comment.
🚨You end up allocating copying bytes twice here. I think what you want to do is this._buf.subarray(0, -1) that will create new Uint8Array view on the same underlying ArrayBuffer, which is what former code seemed to be doing.
Pulls in the latest interface-datastore and replaces use of node Buffers with Uint8Arrays Depends on: - [ ] ipfs/interface-datastore#43 - [ ] ipfs/js-datastore-core#27 - [ ] libp2p/js-libp2p-record#23
Swaps node Buffer for Uint8Array in keys and values.
BREAKING CHANGES:
key.toBufferhas been replaced withkey.uint8Array()