Skip to content

Conversation

@dhendo
Copy link
Contributor

@dhendo dhendo commented Dec 13, 2017

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
Copy link
Contributor Author

dhendo commented Dec 14, 2017

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

@mcollina
Copy link
Owner

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

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

@dhendo
Copy link
Contributor Author

dhendo commented Dec 15, 2017

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?

@dhendo dhendo closed this Dec 15, 2017
@dhendo dhendo reopened this Dec 15, 2017
@mcollina
Copy link
Owner

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.

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina mcollina merged commit fb14fbd into mcollina:master Dec 21, 2017
@mcollina
Copy link
Owner

Thanks! Released as 4.0.0

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 this pull request may close these issues.

2 participants