mcollina / msgpack5

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

Added support for the timestamp extension #60

Closed dhendo closed 6 years ago

dhendo commented 6 years ago

I've added basic support for the timestamp extension type to msgpack5.

This does not support Timestamp96, and in Timestamp64 the nanoseconds will be truncated to millis to mirror the Javascript Date()

@mcollina The tests (&standard) passed, but the pre-commit hook fails, for no visible reason that I could see - any ideas? not ok 4117 no plan foundame ⨯ fail 1

dhendo commented 6 years ago

Thinking on it, encoding probably wants to sit behind an options flag, as users may already be encoding Dates manually. Thoughts?

mcollina commented 6 years ago

Which version of Node and operating system are you running this?

I think we can bump the major and start encoding dates ourselves.

dhendo commented 6 years ago

Ubuntu 14.04, v6.11.5

Testing seems a little unpredictable - it generally passes for me using npm test: image

But I've had failures (after exlcluding the timestamp tests): image

And also passes (after a few attempts). Note the different number of tests:

image

A major version bump sounds good, but would we still need to allow the user to set an option to allow the date encoding to be bypassed, so they can fall through to the

else if (typeof obj === 'object') { buf = encodeExt(obj) || encodeObject(obj)

branch - useful in the situation where they are talking to a decoder that does not support the timestamp extension / or they wish to encode dates in a custom way?

mcollina commented 6 years ago

Can you add such an option? Not sure what is happening in the tests really - if you figure it out feel free to add a fix.

CI is failing on Node 0.10. Feel free to remove that and 0.12 too from .travis.yml.

mcollina commented 6 years ago

Thanks! Released as 4.0.0