search

bufbuild / protobuf-es

Protocol Buffers for ECMAScript. The only JavaScript Protobuf library that is fully-compliant with Protobuf conformance tests.
Apache License 2.0
1.16k stars 69 forks source link

Add message and field name to binary serialization errors #984

Open bhollis opened 1 month ago

bhollis commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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)
timostamm commented 1 month ago

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