mcollina / msgpack5

A msgpack v5 implementation for node.js, with extension points / msgpack.org[Node]
MIT License
492 stars 76 forks source link

`instanceof bl` does not work across versions of `bufferlist` #104

Closed everett1992 closed 2 years ago

everett1992 commented 2 years ago

msgpack5 has a dependency on bl@^4.0.0 If a msgpack5 consumer passes a BufferList from a different bl module

https://github.com/mcollina/msgpack5/blob/master/lib/decoder.js#L45-L48

msgpack5 uses instanceof require('bl') to check if it's argument is a BufferList which will return false if the consumer resolves a different bl package. This happens when msgpack5 and the consumer depend on different versions of bufferlist.

node_modules/
  bl/ , require('bl') in consumer code
  msgpack5/
    node_modules/
      bl/ <- require('bl') in msgpack5 code

When I upgraded my applications bl to 5 tests started failing because the BufferList passed to decode was not consumed.

const { decode, encode } = require('msgpack5')()
const bl = new BufferList([encode(0), encode(1), encode(2)])
assert(bl.length === 3)
decode(bl)
// fails after upgrading to bl@5.
assert(bl.length === 2)

I have a few suggestions

mcollina commented 2 years ago

The latter would be better! Would you like to send a PR?

everett1992 commented 2 years ago

Sent #105 before I saw this response, I did both but I can back out the peerDep change. Why not use a peer dep?