Skip to content
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

instanceof bl does not work across versions of bufferlist #104

Closed
everett1992 opened this issue Apr 11, 2022 · 2 comments · Fixed by #105
Closed

instanceof bl does not work across versions of bufferlist #104

everett1992 opened this issue Apr 11, 2022 · 2 comments · Fixed by #105

Comments

@everett1992
Copy link
Contributor

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

  • move bl to a peer dependency
  • use bl@5's BufferList.isBufferList method which uses a symbol on the bl instance and should work across versions.
everett1992 pushed a commit to everett1992/msgpack5 that referenced this issue Apr 11, 2022
`instanceof` doesn't work when using different instances of the
BufferList class, for example when different versions of buffer list are
used.

This commit upgrades to bl@5 which adds `isBufferList` which should work
across versions of `bl`.

I also moved `bl` to a peer dependency so it's easier for consumers to
match the bl version, and bumped the version to 6.0.0 because npm
doesn't install peer dependencies by default which could break consumers
without a bl dependency.

fixes mcollina#104
@mcollina
Copy link
Owner

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

@everett1992
Copy link
Contributor Author

everett1992 commented Apr 11, 2022

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

everett1992 pushed a commit to everett1992/msgpack5 that referenced this issue Apr 11, 2022
`instanceof` doesn't work when using different instances of the
BufferList class, for example when different versions of buffer list are
used.

This commit upgrades to bl@5 which adds `isBufferList` which should work
across versions of `bl`.

fixes mcollina#104
mcollina pushed a commit that referenced this issue Apr 12, 2022
`instanceof` doesn't work when using different instances of the
BufferList class, for example when different versions of buffer list are
used.

This commit upgrades to bl@5 which adds `isBufferList` which should work
across versions of `bl`.

fixes #104

Co-authored-by: Caleb ツ Everett <calebev@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants