search

darrachequesne / notepack

A fast Node.js implementation of the latest MessagePack spec
MIT License
75 stars 19 forks source link

toJSON method support #7

Closed roeycohen closed 7 years ago

roeycohen commented 7 years ago

added to the encoder support for objects with toJSON method.

roeycohen commented 7 years ago

i'm not sure how to fix this... are the tests using objects with toJSON method?

darrachequesne commented 7 years ago

I think that's because Buffer has a toJSON method. How about moving it after the Buffer and ArrayBuffer checks there?

Also, shouldn't it rather be return _encode(value.toJSON())? Besides, could you please add a test case, and also update the browser file here? Thanks!

roeycohen commented 7 years ago

ok, i've made the change. now all tests are passed.

toJSON might return Buffers and Dates and all kind of things and i wanted it to support these cases...

roeycohen commented 7 years ago

return _encode(value.toJSON())

this is the same as putting the toJSON at the beginning of the _encode method and we also have the initial call from encode.

i'll write a few more tests and push again.

roeycohen commented 7 years ago

ok, added a new test. hope now everything is ready for merging :)

roeycohen commented 7 years ago

well that's weird... i've only changed indentation in test file...

darrachequesne commented 7 years ago

@roeycohen I implemented it a bit differently, please see https://github.com/darrachequesne/notepack/pull/8

I think the failure was due to a timeout during the tests, so I increased the value here.

Again, thanks :+1:

roeycohen commented 7 years ago

seems great :) to bad i didn't notice the "return" call at the end of each type...

roeycohen commented 7 years ago

it will be nice to update the package.json file of https://github.com/socketio/socket.io-redis/blob/master/package.json to the latest release of this plugin.