-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add message and field name to binary serialization errors #984
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR, Ben. We'll need some insight whether / how this impacts performance and bundle size. I think it would be ideal to use the same mechanism for error messages that |
Running Edit: Hmm, looks like there aren't any performance tests for Before:
After:
|
7dee7dd
to
624cc52
Compare
@@ -95,173 +102,163 @@ interface Test { | |||
} | |||
|
|||
function setupTests(): Test[] { | |||
const tests: Test[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a huge change but I'm just switching to generating the benchmark cases instead of having to repeat code.
return [ | ||
{ | ||
name: `fromBinary ${name}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual change - we now benchmark fromBinary
, fromJsonString
, toBinary
, and toJsonString
for each example message.
Thanks for the updates, Ben. I'll take a look as soon as possible, but have to take care of some Connect work first. |
This adds the message and field name to errors thrown during binary serialization of scalar fields. It should convert a message like:
To:
I followed the wording of the error thrown here for the phrasing: https://github.com/bufbuild/protobuf-es/blob/main/packages/protobuf/src/to-binary.ts#L74
Fixes #983