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

new msgpack spec compatibility #6

Merged
merged 8 commits into from
Jun 9, 2015

Conversation

ambrusc
Copy link
Contributor

@ambrusc ambrusc commented Jun 6, 2015

Hi Tim,

The msgpack spec added a Bin type and got an updated spec back in August 2013. I'd like to update the msgpack.js encoder and decoder to support the new spec.

Unfortunately, some of the existing extensions used reserved data type which are part of the new spec:

  • 0xc4 (previously encoded undefined) is now specced as bin 8
  • 0xd8 (previously a buffer type) is now specced as fixext 16
  • 0xd9 (previously a buffer type) is now specced as str 8

I've had to change the encoding/decoding which breaks backwards-compatibility with previous msgpack-js encoded data if they used the reserved types.

Do you think we can merge this change to be compatible with the updated spec? Any suggestions on how to handle backwards compatibility with the js-specific extensions?

~Ambrus

@creationix
Copy link
Owner

I'm fine with breaking backwards compat. We just need to bump the major version in npm.

@ambrusc
Copy link
Contributor Author

ambrusc commented Jun 9, 2015

Thanks Tim, I've updated package.json do we also need to update component.json? Apologies I've never used component.json files so I don't know that they're for. Additionally the project has a different name there (msgpack-js). Is that intentional?

@creationix
Copy link
Owner

I don't think the package manager component.js was written for is used much these days. We can bump the version the same though, it won't hurt.

@ambrusc
Copy link
Contributor Author

ambrusc commented Jun 9, 2015

Thanks for taking the time to review; much appreciated.

creationix added a commit that referenced this pull request Jun 9, 2015
new msgpack spec compatibility
@creationix creationix merged commit b2e1803 into creationix:master Jun 9, 2015
@creationix
Copy link
Owner

Oh I forgot, the README needs updating. We are no longer extending the format and are instead using the latest version of the spec right?

@creationix
Copy link
Owner

And if you're feeling really adventurous, you can help me with creationix/msgpack-js#8

@ambrusc
Copy link
Contributor Author

ambrusc commented Jun 9, 2015

The new spec (https://github.com/msgpack/msgpack/blob/master/spec.md) allows for "application specific extension types". There's no registry of typecodes so there's not too much hope of extension types being cross-platform. In light of that, I opted to use 0x00 to encode undefined. (Comically I just noticed creationix/msgpack-js#8 opted to use the same encoding I did).

Other than that, binary types are now part of the spec so the rest is vanilla and complete as far as I know.

On a different note, I hadn't yet given much thought to allowing the user to decode extension types via a callback, but that would probably be useful.

I was going to wait and use this version for a week or so to see if I found any obvious problems before proposing a change to msgpack-js. Mind if we let this simmer for a bit before I take a look at that PR?

@ambrusc ambrusc deleted the msgpack-spec branch June 9, 2015 17:45
@creationix
Copy link
Owner

No rush, let me know when you're ready.

@creationix creationix mentioned this pull request Jul 8, 2015
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