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

feat: msgpack encoding #3460

Merged
merged 27 commits into from
Jul 10, 2023
Merged

feat: msgpack encoding #3460

merged 27 commits into from
Jul 10, 2023

Conversation

lino-levan
Copy link
Contributor

Closes #2194.

@lino-levan
Copy link
Contributor Author

I should note that I plan for at least one follow-up PR. We don't support the timestamp extension to the spec yet, but I think that's fine for making the first PR reviewable.

Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Nice work in general! Needs a bit of work.

msgpack/decode.ts Outdated Show resolved Hide resolved
msgpack/decode.ts Outdated Show resolved Hide resolved
@lino-levan lino-levan requested a review from aapoalas June 23, 2023 16:37
@nmarks413
Copy link

lgtm

@idranme
Copy link

idranme commented Jun 25, 2023

@aapoalas What do you think?

@aapoalas
Copy link
Collaborator

I'll try to review this fully in some hours.

@lino-levan
Copy link
Contributor Author

How am I failing CI if main isn't? Super strange.

Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Found some nits to pick and a few remaining issues from the encode side.

msgpack/decode.ts Show resolved Hide resolved
msgpack/decode.ts Outdated Show resolved Hide resolved
msgpack/decode.ts Outdated Show resolved Hide resolved
msgpack/decode.ts Outdated Show resolved Hide resolved
msgpack/decode_test.ts Show resolved Hide resolved
msgpack/encode.ts Show resolved Hide resolved
msgpack/encode.ts Show resolved Hide resolved
msgpack/encode.ts Outdated Show resolved Hide resolved
msgpack/encode.ts Outdated Show resolved Hide resolved
msgpack/encode.ts Show resolved Hide resolved
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Nice work! I think in a followup we could try to optimise the encoding side via eg. eagerly allocating a larger buffer that we use as needed and reallocate a bigger one if needed. Or we could precalculate the size we need (strings would need to be "eyeballed" or allocated to the theoretical maximum of length * 3).

Also, an "encodeInto" API is probably a good idea in the future.

@lino-levan
Copy link
Contributor Author

Lots of good ideas for follow up PRs. Copying this from discord so that we have all of the ideas in the same thread:

Later we'll probably want encode options in the vein of:

  • BigInt representation: Always 64 bit / Size-optimized
  • Number representation: Always f64 / Size-optimized
  • Undefined representation: Not allowed / Use null / Ignore in maps

And decode options to go with them:

  • 64-bit integers: Mixed / Always BigInt / Always number (unsafe)
  • Small integers: Number / BigInt

@lino-levan
Copy link
Contributor Author

Let me finish up some final cleanups around comments and bitshift stuff and I think this PR is good.

@lino-levan
Copy link
Contributor Author

@kt3k please take a look!

@idranme
Copy link

idranme commented Jul 4, 2023

@kt3k What do you think?

return value;
}
case 0xcf: { // uint 64
const value = dataView.getBigUint64(pointer.consumed);
Copy link
Member

@kt3k kt3k Jul 6, 2023

Choose a reason for hiding this comment

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

Does this mean integer type in msgpack could become both number and bigint in JS? Is this the same as the other decoders such as msgpackr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this mean integer type in msgpack could become both number and bigint in JS?

There are multiple integer types, but yes. Some integer types (u64 + i64) will be converted to a bigint instead of a number.

Is this the same as the other decoders such as msgpackr?

It appears that msgpack does the same thing (always storing it in a u64/i64 and erroring out if the number is too big)
image

msgpack/decode.ts Outdated Show resolved Hide resolved
lino-levan and others added 2 commits July 6, 2023 07:04
msgpack/encode.ts Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Jul 7, 2023

Let's add jsdocs to encode and decode with examples. Those will be main docs for this module.

@kt3k
Copy link
Member

kt3k commented Jul 7, 2023

Other part looks good to me

@lino-levan lino-levan requested a review from kt3k July 7, 2023 14:00
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Looks nice addition to std. Thanks for your contribution! @lino-levan

@kt3k kt3k merged commit 642a111 into denoland:main Jul 10, 2023
@lino-levan lino-levan deleted the feat-msgpack branch August 31, 2023 04:33
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.

Add MessagePack's encode and decode
5 participants