V2: Throw errors in ReflectMessage instead of returning them (#882)

Author: timostamm

packages/bundle-size/README.md

@@ -16,11 +16,11 @@ usually do. We repeat this for an increasing number of files.
 <!--- TABLE-START -->
 | code generator  | files    | bundle size             | minified               | compressed         |
 |-----------------|----------|------------------------:|-----------------------:|-------------------:|
-| protobuf-es | 1 | 123,350 b | 64,136 b | 14,970 b |
-| protobuf-es | 4 | 125,545 b | 65,646 b | 15,662 b |
-| protobuf-es | 8 | 128,323 b | 67,417 b | 16,196 b |
-| protobuf-es | 16 | 138,831 b | 75,398 b | 18,504 b |
-| protobuf-es | 32 | 166,726 b | 97,413 b | 23,953 b |
+| protobuf-es | 1 | 123,305 b | 64,143 b | 15,010 b |
+| protobuf-es | 4 | 125,500 b | 65,653 b | 15,656 b |
+| protobuf-es | 8 | 128,278 b | 67,424 b | 16,182 b |
+| protobuf-es | 16 | 138,786 b | 75,405 b | 18,508 b |
+| protobuf-es | 32 | 166,681 b | 97,420 b | 23,963 b |
 | protobuf-javascript | 1 | 339,613 b | 255,820 b | 42,481 b |
 | protobuf-javascript | 4 | 366,281 b | 271,092 b | 43,912 b |
 | protobuf-javascript | 8 | 388,324 b | 283,409 b | 45,038 b |

packages/bundle-size/chart.svg

@@ -16,8 +16,20 @@ import { UpstreamProtobuf } from "upstream-protobuf";
 import { fromBinary, createFileRegistry } from "@bufbuild/protobuf";
 import type { FileDescriptorSet } from "@bufbuild/protobuf/wkt";
 import { FileDescriptorSetDesc } from "@bufbuild/protobuf/wkt";
+import { FieldError } from "@bufbuild/protobuf/reflect";
 import assert from "node:assert";
 
+export function catchFieldError(fn: () => unknown): FieldError | undefined {
+  try {
+    fn();
+  } catch (e) {
+    if (e instanceof FieldError) {
+      return e;
+    }
+  }
+  return undefined;
+}
+
 let upstreamProtobuf: UpstreamProtobuf | undefined;
 
 export async function compileFileDescriptorSet(

packages/protobuf-test/src/helpers.ts

@@ -23,6 +23,7 @@ import * as proto3_ts from "../gen/ts/extra/proto3_pb.js";
 import { create, protoInt64 } from "@bufbuild/protobuf";
 import { UserDesc } from "../gen/ts/extra/example_pb.js";
 import assert from "node:assert";
+import { catchFieldError } from "../helpers.js";
 
 describe("reflectList()", () => {
   test("creates ReflectList", () => {
@@ -95,87 +96,88 @@ describe("ReflectList", () => {
     test("adds item", () => {
       const local: unknown[] = ["a"];
       const list = reflectList(repeatedStringField, local);
-      expect(list.add("b")).toBeUndefined();
-      expect(list.add("c")).toBeUndefined();
+      list.add("b");
+      list.add("c");
       expect(local).toStrictEqual(["a", "b", "c"]);
     });
     test("adds items", () => {
       const local: unknown[] = [];
       const list = reflectList(repeatedStringField, local);
-      expect(list.add("a", "b", "c")).toBeUndefined();
+      list.add("a", "b", "c");
       expect(local).toStrictEqual(["a", "b", "c"]);
     });
     test("does not add any item if one of several items is invalid", () => {
       const local: unknown[] = [];
       const list = reflectList(repeatedStringField, local);
-      expect(list.add("a", "b", true)).toBeDefined();
+      const err = catchFieldError(() => list.add("a", "b", true));
+      expect(err).toBeDefined();
       expect(local).toStrictEqual([]);
     });
     test("converts number, string, bigint to bigint for 64-bit integer field", () => {
       const local: unknown[] = [];
       const list = reflectList(repeatedInt64Field, local);
-      expect(list.add(1)).toBeUndefined();
-      expect(list.add("2")).toBeUndefined();
-      expect(list.add(n3)).toBeUndefined();
+      list.add(1);
+      list.add("2");
+      list.add(n3);
       expect(local).toStrictEqual([n1, n2, n3]);
     });
     test("converts number, string, bigint to string for 64-bit integer field with jstype=JS_STRING", () => {
       const local: unknown[] = [];
       const list = reflectList(repeatedInt64JsStringField, local);
-      expect(list.add(1)).toBeUndefined();
-      expect(list.add("2")).toBeUndefined();
-      expect(list.add(n3)).toBeUndefined();
+      list.add(1);
+      list.add("2");
+      list.add(n3);
       expect(local).toStrictEqual(["1", "2", "3"]);
     });
     test("returns error for wrong message type", () => {
       const list = reflectList(repeatedMessageField, []);
-      const err = list.add(reflect(UserDesc));
+      const err = catchFieldError(() => list.add(reflect(UserDesc)));
       expect(err?.message).toMatch(
         /^list item #1: expected ReflectMessage \(spec.Proto3Message\), got ReflectMessage \(docs.User\)$/,
       );
     });
     test("returns error for invalid scalar", () => {
       const list = reflectList(repeatedStringField, []);
-      const err = list.add(true);
+      const err = catchFieldError(() => list.add(true));
       expect(err?.message).toMatch(/^list item #1: expected string, got true$/);
     });
   });
   describe("set()", () => {
     test("replaces item at index", () => {
       const local: unknown[] = ["a", "b"];
       const list = reflectList(repeatedStringField, local);
-      expect(list.set(0, "c")).toBeUndefined();
+      list.set(0, "c");
       expect(local).toStrictEqual(["c", "b"]);
     });
     test("converts number, string, bigint to bigint for 64-bit integer field", () => {
       const local: unknown[] = [n0, n0, n0];
       const list = reflectList(repeatedInt64Field, local);
-      expect(list.set(0, 1)).toBeUndefined();
-      expect(list.set(1, "2")).toBeUndefined();
-      expect(list.set(2, n3)).toBeUndefined();
+      list.set(0, 1);
+      list.set(1, "2");
+      list.set(2, n3);
       expect(local).toStrictEqual([n1, n2, n3]);
     });
     test("converts number, string, bigint to string for 64-bit integer field with jstype=JS_STRING", () => {
       const local: unknown[] = ["0", "0", "0"];
       const list = reflectList(repeatedInt64JsStringField, local);
-      expect(list.set(0, 1)).toBeUndefined();
-      expect(list.set(1, "2")).toBeUndefined();
-      expect(list.set(2, n3)).toBeUndefined();
+      list.set(0, 1);
+      list.set(1, "2");
+      list.set(2, n3);
       expect(local).toStrictEqual(["1", "2", "3"]);
     });
-    test("returns error if out of range", () => {
+    test("throws error if out of range", () => {
       const list = reflectList(repeatedStringField, []);
-      const err = list.set(0, "abc");
+      const err = catchFieldError(() => list.set(0, "abc"));
       expect(err?.message).toMatch(/^list item #1: out of range$/);
     });
-    test("returns error for invalid scalar", () => {
+    test("throws error for invalid scalar", () => {
       const list = reflectList(repeatedStringField, [null]);
-      const err = list.set(0, true);
+      const err = catchFieldError(() => list.set(0, true));
       expect(err?.message).toMatch(/^list item #1: expected string, got true$/);
     });
-    test("returns error for wrong message type", () => {
+    test("throws error for wrong message type", () => {
       const list = reflectList(repeatedMessageField, [null]);
-      const err = list.set(0, reflect(UserDesc));
+      const err = catchFieldError(() => list.set(0, reflect(UserDesc)));
       expect(err?.message).toMatch(
         /^list item #1: expected ReflectMessage \(spec.Proto3Message\), got ReflectMessage \(docs.User\)$/,
       );

packages/protobuf-test/src/reflect/reflect-list.test.ts

@@ -24,6 +24,7 @@ import { protoInt64 } from "@bufbuild/protobuf";
 import { UserDesc } from "../gen/ts/extra/example_pb.js";
 import { create } from "@bufbuild/protobuf";
 import assert from "node:assert";
+import { catchFieldError } from "../helpers.js";
 
 describe("reflectMap()", () => {
   test("creates ReflectMap", () => {
@@ -166,15 +167,15 @@ describe("ReflectMap", () => {
     test("sets entry", () => {
       const local = {};
       const map = reflectMap(mapStringStringField, local);
-      expect(map.set("a", "A")).toBeUndefined();
+      map.set("a", "A");
       expect(local).toStrictEqual({ a: "A" });
     });
     test("converts key", () => {
       const local = {};
       const map = reflectMap(mapInt64Int64Field, local);
-      expect(map.set(1, n11)).toBeUndefined();
-      expect(map.set(n2, n22)).toBeUndefined();
-      expect(map.set("3", n33)).toBeUndefined();
+      map.set(1, n11);
+      map.set(n2, n22);
+      map.set("3", n33);
       expect(local).toStrictEqual({
         "1": n11,
         "2": n22,
@@ -184,32 +185,32 @@ describe("ReflectMap", () => {
     test("converts long value", () => {
       const local = {};
       const map = reflectMap(mapInt64Int64Field, local);
-      expect(map.set(n1, n11)).toBeUndefined();
-      expect(map.set(n2, 22)).toBeUndefined();
-      expect(map.set(n3, "33")).toBeUndefined();
+      map.set(n1, n11);
+      map.set(n2, n22);
+      map.set(n3, n33);
       expect(local).toStrictEqual({
         "1": n11,
         "2": n22,
         "3": n33,
       });
     });
-    test("returns error for invalid key", () => {
+    test("throws error for invalid key", () => {
       const map = reflectMap(mapInt32Int32Field, {});
-      const err = map.set(true, "A");
+      const err = catchFieldError(() => map.set(true, "A"));
       expect(err?.message).toMatch(
         /^invalid map key: expected number \(int32\), got true$/,
       );
     });
-    test("returns error for invalid scalar value", () => {
+    test("throws error for invalid scalar value", () => {
       const map = reflectMap(mapStringStringField, {});
-      const err = map.set("a", true);
+      const err = catchFieldError(() => map.set("a", true));
       expect(err?.message).toMatch(
         /^map entry "a": expected string, got true$/,
       );
     });
-    test("returns error for wrong message type", () => {
+    test("throws error for wrong message type", () => {
       const map = reflectMap(mapInt32MessageField, {});
-      const err = map.set(1, reflect(UserDesc));
+      const err = catchFieldError(() => map.set(1, reflect(UserDesc)));
       expect(err?.message).toMatch(
         /^map entry 1: expected ReflectMessage \(spec.Proto3Message\), got ReflectMessage \(docs.User\)$/,
       );