search

mcollina / msgpack5

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

Update decoder.js [timestamp bugfix] #65

Closed etinin closed 6 years ago

etinin commented 6 years ago

[FIXED] Lack of offsets presents errors when trying to decode timestamps.

mcollina commented 6 years ago

Can you add unit tests?

etinin commented 6 years ago

I will take a look at that as soon as I have some time.

The proper way to test this would be to implement browser testing, because although the official node API doesn't mark the offset parameter as optional, it apparently has a default value of zero. I think the current tests should suffice to demonstrate this issue, as long as they are run on the browser.

The browser implementation feross/buffer is different (at least in the version used by the latest webpack), though and will throw an error if you attempt to invoke any of the read functions without an offset specified. Since Node's API spec does not guarantee this behavior of having 0 as the default offset, I believe this should be fixed on msgpack5's end.

https://nodejs.org/api/buffer.html#buffer_buf_readuint32be_offset_noassert

It's important to implement tests targetting browsers, especially since aspnet/SignalR signalr-protocol-msgpack now makes use of msgpack5.

mcollina commented 6 years ago

We can use https://github.com/airtap/airtap for that.

etinin commented 6 years ago

This particular fix has been implemented by commit 18235ceb88d5b54257184df3f21b046ceef9c0st, which is identical to this pull request.