feat: add maxLength to allow controlling max buffer length during decode #9
Conversation
|
One more thing, I have tentatively set |
src/decode.js
Outdated
| @@ -10,6 +10,7 @@ exports.decodeFromReader = decodeFromReader | |||
|
|
|||
| const MSB = 0x80 | |||
| const isEndByte = (byte) => !(byte & MSB) | |||
| const MAX_LENGTH = 1024 | |||
There was a problem hiding this comment.
go currently used 4mb as the max length, so lets make that the default
|
@dryajov I've updated master to have passing tests again. Please rebase onto it so I can merge |
|
@dignifiedquire thanks, I'll update the length! |
|
@dignifiedquire think this is ready now? |
README.md
Outdated
| @@ -70,6 +70,7 @@ Returns a pull-stream through. | |||
| - `opts: Object`, optional | |||
| - `fixed: false`: | |||
| - `bytes: 4`: If `fixed` is `true` this is the amount of bytes used for the prefix. | |||
| - `maxLength`: If provided, will not decode messages longer than the size specified | |||
There was a problem hiding this comment.
this should mention the default length
test/index.spec.js
Outdated
| encoded | ||
| ).to.be.eql([ | ||
| Buffer.concat([ | ||
| new Buffer(varint.encode('hello '.length)), |
There was a problem hiding this comment.
please use Buffer.from instead
There was a problem hiding this comment.
Should I just make the change everywhere in the tests while I'm at it?
There was a problem hiding this comment.
yeah, also that will require using safe-buffer, for node 4 compat
|
@dignifiedquire any reason to not just do |
|
tests are failing on node 4 even tho I'm pulling |
|
Seems like in node 4, if the fill value is something other than a string and no encoding is explicitly specified, then it will not encode it with anything and simply fill in the raw bytes - https:/nodejs/node/blob/v4.x/lib/buffer.js#L109, which is different from how later versions behave, where buffer is also defaulted to 'utf8'. |
package.json
Outdated
| @@ -32,6 +32,7 @@ | |||
| }, | |||
| "homepage": "https:/dignifiedquire/pull-length-prefixed#readme", | |||
| "dependencies": { | |||
| "mocha": "^3.2.0", | |||
There was a problem hiding this comment.
Ah, thanks for catching that, leftovers from my tests figuring out what's wrong with node 4.
|
@dignifiedquire can we get this published to npm please 😄? This is needed for libp2p/js-libp2p-circuit#9 |
|
sorry for the delay, published a new version @dryajov |
@dignifiedquire adding this since we need to control the max msg size in circuit.
FYI, some tests are failing, but they were failing before my changes, I haven't yet taken a look at it, will do soon, but you might know whats going on.