Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Jun 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/bundle-size/README.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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