Skip to content

Commit

Permalink
V2: Throw errors in ReflectMessage instead of returning them (#882)
Browse files Browse the repository at this point in the history
  • Loading branch information
@timostamm
timostamm authored Jun 6, 2024
1 parent a81603e commit 4d90100
Show file tree
Hide file tree
Showing 10 changed files with 133 additions and 151 deletions.
10 changes: 5 additions & 5 deletions packages/bundle-size/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
12 changes: 6 additions & 6 deletions packages/bundle-size/chart.svg
View file
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions packages/protobuf-test/src/helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
52 changes: 27 additions & 25 deletions packages/protobuf-test/src/reflect/reflect-list.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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\)$/,
);
Expand Down
27 changes: 14 additions & 13 deletions packages/protobuf-test/src/reflect/reflect-map.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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,
Expand All @@ -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\)$/,
);
Expand Down
Loading

0 comments on commit 4d90100

Please sign in to comment.