Add message and field name to binary serialization errors

[bhollis]

This adds the message and field name to errors thrown during binary serialization of scalar fields. It should convert a message like:

Error: invalid uint32: -1

To:

Error: cannot encode field com.whatever.MyMessage.my_field to binary: invalid uint32: -1

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

[timostamm]

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 toJson uses (but writeField is also used for extensions). We need good test coverage to make sure error messages have the expected content and will not regress.

[bhollis]

Running npm run perf in packages/protobuf-test:

Edit: Hmm, looks like there aren't any performance tests for toBinary. Let me add them.

Before:

toBinary   perf-payload.bin x 5,528 ops/sec ±4.33% (94 runs sampled)
toBinary   tiny example.User x 1,098,031 ops/sec ±0.35% (95 runs sampled)
toBinary   normal example.User x 177,436 ops/sec ±1.62% (95 runs sampled)
toBinary   scalar values x 275,100 ops/sec ±0.76% (97 runs sampled)
toBinary   repeated scalar values x 94,595 ops/sec ±0.64% (98 runs sampled)
toBinary   map with scalar keys and values x 65,719 ops/sec ±0.58% (95 runs sampled)
toBinary   repeated field with 1000 messages x 2,984 ops/sec ±0.56% (98 runs sampled)
toBinary   map field with 1000 messages x 729 ops/sec ±0.71% (97 runs sampled)

After:

toBinary   perf-payload.bin x 5,452 ops/sec ±1.57% (97 runs sampled)
toBinary   tiny example.User x 1,076,997 ops/sec ±0.32% (96 runs sampled)
toBinary   normal example.User x 182,993 ops/sec ±0.56% (98 runs sampled)
toBinary   scalar values x 277,949 ops/sec ±0.55% (95 runs sampled)
toBinary   repeated scalar values x 90,580 ops/sec ±3.47% (92 runs sampled)
toBinary   map with scalar keys and values x 63,867 ops/sec ±0.62% (95 runs sampled)
toBinary   repeated field with 1000 messages x 2,956 ops/sec ±0.51% (97 runs sampled)
toBinary   map field with 1000 messages x 696 ops/sec ±0.79% (94 runs sampled)

[bhollis]

This looks like a huge change but I'm just switching to generating the benchmark cases instead of having to repeat code.

[bhollis]

This is the actual change - we now benchmark fromBinary, fromJsonString, toBinary, and toJsonString for each example message.

[timostamm]

Thanks for the updates, Ben. I'll take a look as soon as possible, but have to take care of some Connect work first.

[bhollis]

A gentle nudge on this - it would still be of great benefit to have this information.

[timostamm]

A gentle nudge on this - it would still be of great benefit to have this information.

Apologies for the delay, Ben.

It doesn't seem ideal that we have to pass message and field name around. It would be great if we could use checkField from reflect-check.ts like toJson does, but I wasn't able to find a way to utilize it without larger refactors.

We may extend and improve reflect-check.ts in the near future for protovalidate, and it's possible that this gives us new options to tighten up the code and also improve error messages. I don't think it's useful to wait for that though, and I think it makes sense to merge this improvement.

Two minor things though, before we merge:

• Let's include Add more tests for serialization errors #1051.
• Fields have a reference to the message they are defined in - we can use that instead of passing the message name. See comments below.

[bhollis]

Sure, here are those changes.

[timostamm]

Thanks!