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

Jump to bottom

V2: Remove addListItem and setMapEntry from ReflectMessage #875

Merged
merged 2 commits into from
Jun 3, 2024
Merged

V2: Remove addListItem and setMapEntry from ReflectMessage #875

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b4aefb
Remove addListItem and setMapEntry from ReflectMessage
timostamm Jun 3, 2024
7347a96
Merge remote-tracking branch 'origin/v2' into tstamm/simplify-reflect
timostamm Jun 3, 2024
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,775 b | 64,241 b | 14,974 b |
| protobuf-es | 4 | 125,970 b | 65,745 b | 15,652 b |
| protobuf-es | 8 | 128,748 b | 67,516 b | 16,179 b |
| protobuf-es | 16 | 139,256 b | 75,497 b | 18,508 b |
| protobuf-es | 32 | 167,151 b | 97,519 b | 23,990 b |
| protobuf-es | 1 | 123,004 b | 63,889 b | 14,924 b |
| protobuf-es | 4 | 125,199 b | 65,399 b | 15,584 b |
| protobuf-es | 8 | 127,977 b | 67,170 b | 16,092 b |
| protobuf-es | 16 | 138,485 b | 75,151 b | 18,372 b |
| protobuf-es | 32 | 166,380 b | 97,166 b | 23,862 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.
201 changes: 0 additions & 201 deletions packages/protobuf-test/src/reflect/reflect.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,205 +608,4 @@ describe("ReflectMessage", () => {
);
});
});
describe("addListItem()", () => {
const desc = proto3_ts.Proto3MessageDesc;
let msg: proto3_ts.Proto3Message;
let r: ReflectMessage;
beforeEach(() => {
msg = create(desc);
r = reflect(desc, msg);
});
test("adds valid item to repeatedStringField", () => {
const f = desc.field.repeatedStringField;
assert(f.fieldKind == "list");
const err = r.addListItem(f, "abc");
expect(err).toBeUndefined();
expect(msg.repeatedStringField).toStrictEqual(["abc"]);
});
test("adds unknown value for open enum", () => {
const f = desc.field.repeatedEnumField;
assert(f.fieldKind == "list");
const err = r.addListItem(f, 99);
expect(err).toBeUndefined();
expect(msg.repeatedEnumField).toStrictEqual([99]);
});
test("adds bigint, number, and string as bigint", () => {
const f = desc.field.repeatedInt64Field;
assert(f.fieldKind == "list");
r.addListItem(f, protoInt64.parse(1));
r.addListItem(f, 2);
r.addListItem(f, "3");
expect(msg.repeatedInt64Field).toStrictEqual([
protoInt64.parse(1),
protoInt64.parse(2),
protoInt64.parse(3),
]);
});
test("adds bigint, number, and string as string for jstype=JS_STRING", () => {
const f = desc.field.repeatedInt64JsStringField;
assert(f.fieldKind == "list");
r.addListItem(f, protoInt64.parse(1));
r.addListItem(f, 2);
r.addListItem(f, "3");
expect(msg.repeatedInt64JsStringField).toStrictEqual(["1", "2", "3"]);
});
test("throws error on foreign field", async () => {
const foreignMessage = await compileMessage(`
syntax="proto3";
message Foreign { repeated string foreign = 1;}
`);
const foreignField = foreignMessage.fields[0];
assert(foreignField.fieldKind == "list");
expect(() => r.addListItem(foreignField, "value")).toThrow(
/^cannot use field Foreign.foreign with message spec.Proto3Message$/,
);
});
describe("returns error on invalid item", () => {
test("bool for repeatedStringField", () => {
const f = desc.field.repeatedStringField;
assert(f.fieldKind == "list");
const err = r.addListItem(f, true);
expect(err?.message).toMatch(
/^list item #1: expected string, got true$/,
);
expect(err?.name).toMatch("FieldValueInvalidError");
});
test("number out of range for repeatedInt32Field", () => {
const f = desc.field.repeatedInt32Field;
assert(f.fieldKind == "list");
const err = r.addListItem(f, Number.MAX_SAFE_INTEGER);
expect(err?.message).toMatch(
/^list item #1: expected number \(int32\): 9007199254740991 out of range/,
);
expect(err?.name).toMatch("FieldValueInvalidError");
});
test("message for repeatedMessageField", () => {
const f = desc.field.repeatedMessageField;
assert(f.fieldKind == "list");
// @ts-expect-error ignore to test runtime behavior
const err = r.addListItem(f, create(example_ts.UserDesc));
expect(err?.message).toMatch(
/^list item #1: expected ReflectMessage \(spec.Proto3Message\), got message docs.User/,
);
expect(err?.name).toMatch("FieldValueInvalidError");
});
test("wrong ReflectMessage for repeatedMessageField", () => {
const f = desc.field.repeatedMessageField;
assert(f.fieldKind == "list");
const testMessage = reflect(example_ts.UserDesc);
const err = r.addListItem(f, testMessage);
expect(err?.message).toMatch(
/^list item #1: expected ReflectMessage \(spec.Proto3Message\), got ReflectMessage \(docs.User\)/,
);
expect(err?.name).toMatch("FieldValueInvalidError");
});
test("true for repeatedMessageField", () => {
const f = desc.field.repeatedMessageField;
assert(f.fieldKind == "list");
const err = r.addListItem(f, true);
expect(err?.message).toMatch(
/^list item #1: expected ReflectMessage \(spec.Proto3Message\), got true/,
);
expect(err?.name).toMatch("FieldValueInvalidError");
});
});
});
describe("setMapEntry()", () => {
const desc = proto3_ts.Proto3MessageDesc;
let msg: proto3_ts.Proto3Message;
let r: ReflectMessage;
beforeEach(() => {
msg = create(desc);
r = reflect(desc, msg);
});
test("adds valid entry to mapStringStringField", () => {
const f = desc.field.mapStringStringField;
assert(f.fieldKind == "map");
const err = r.setMapEntry(f, "key", "value");
expect(err).toBeUndefined();
expect(msg.mapStringStringField).toStrictEqual({ key: "value" });
});
test("throws error on foreign field", async () => {
const foreignMessage = await compileMessage(`
syntax="proto3";
message Foreign { map<string, string> foreign = 1;}
`);
const foreignField = foreignMessage.fields[0];
if (foreignField.fieldKind != "map") {
throw new Error();
}
expect(() => r.setMapEntry(foreignField, "key", "value")).toThrow(
/^cannot use field Foreign.foreign with message spec.Proto3Message$/,
);
});
test("adds bigint, number, and string value as bigint", () => {
const f = desc.field.mapInt64Int64Field;
assert(f.fieldKind == "map");
expect(
r.setMapEntry(f, protoInt64.parse(1), protoInt64.parse(1)),
).toBeUndefined();
expect(r.setMapEntry(f, protoInt64.parse(2), 2)).toBeUndefined();
expect(r.setMapEntry(f, protoInt64.parse(3), "3")).toBeUndefined();
expect(Object.values(msg.mapInt64Int64Field)).toStrictEqual([
protoInt64.parse(1),
protoInt64.parse(2),
protoInt64.parse(3),
]);
});
test("adds bigint, number, and string key as string", () => {
const f = desc.field.mapInt64Int64Field;
assert(f.fieldKind == "map");
expect(
r.setMapEntry(f, protoInt64.parse(1), protoInt64.parse(1)),
).toBeUndefined();
expect(r.setMapEntry(f, 2, protoInt64.parse(1))).toBeUndefined();
expect(r.setMapEntry(f, "3", protoInt64.parse(1))).toBeUndefined();
expect(Object.keys(msg.mapInt64Int64Field)).toStrictEqual([
"1",
"2",
"3",
]);
});
test("adds bool key as string", () => {
const f = desc.field.mapBoolBoolField;
assert(f.fieldKind == "map");
expect(r.setMapEntry(f, true, true)).toBeUndefined();
expect(r.setMapEntry(f, false, false)).toBeUndefined();
expect(Object.keys(msg.mapBoolBoolField)).toStrictEqual([
"true",
"false",
]);
});
describe("returns error on invalid value", () => {
test("wrong message", () => {
const f = desc.field.mapInt32MessageField;
assert(f.fieldKind == "map");
const err = r.setMapEntry(f, 123, reflect(example_ts.UserDesc));
expect(err?.message).toMatch(
/^map entry 123: expected ReflectMessage \(spec.Proto3Message\), got ReflectMessage \(docs.User\)/,
);
expect(err?.name).toMatch("FieldValueInvalidError");
});
test("number out of range", () => {
const f = desc.field.mapInt32Int32Field;
assert(f.fieldKind == "map");
const err = r.setMapEntry(f, 123, Number.MAX_SAFE_INTEGER);
expect(err?.message).toMatch(
/^map entry 123: expected number \(int32\): 9007199254740991 out of range/,
);
expect(err?.name).toMatch("FieldValueInvalidError");
});
});
describe("returns error on invalid key", () => {
test("number out of range", () => {
const f = desc.field.mapInt32Int32Field;
assert(f.fieldKind == "map");
const err = r.setMapEntry(f, Number.MAX_SAFE_INTEGER, 123);
expect(err?.message).toMatch(
/^invalid map key: expected number \(int32\): 9007199254740991 out of range/,
);
expect(err?.name).toMatch("FieldValueInvalidError");
});
});
});
});
11 changes: 7 additions & 4 deletions packages/protobuf/src/clone.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,21 @@ function cloneReflect(i: ReflectMessage): ReflectMessage {
break;
}
case "list":
// eslint-disable-next-line no-case-declarations
const list = o.get(f);
for (const item of i.get(f)) {
// TODO fix type error
// @ts-expect-error TODO
const err = o.addListItem(f, cloneSingular(f, item));
const err = list.add(cloneSingular(f, item));
if (err) {
throw err;
}
}
break;
case "map":
// eslint-disable-next-line no-case-declarations
const map = o.get(f);
for (const entry of i.get(f).entries()) {
const err = o.setMapEntry(f, entry[0], cloneSingular(f, entry[1]));
// @ts-expect-error TODO fix type error
const err = map.set(entry[0], cloneSingular(f, entry[1]));
if (err) {
throw err;
}
Expand Down
35 changes: 19 additions & 16 deletions packages/protobuf/src/from-binary.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@

import { type DescField, type DescMessage, ScalarType } from "./descriptors.js";
import type { MessageShape } from "./types.js";
import type { MapEntryKey, ReflectMessage } from "./reflect/index.js";
import type {
MapEntryKey,
ReflectList,
ReflectMap,
ReflectMessage,
} from "./reflect/index.js";
import { scalarZeroValue } from "./reflect/scalar.js";
import type { ScalarValue } from "./reflect/scalar.js";
import { reflect } from "./reflect/reflect.js";
Expand Down Expand Up @@ -77,14 +82,15 @@ export function mergeFromBinary<Desc extends DescMessage>(
target: MessageShape<Desc>,
bytes: Uint8Array,
options?: Partial<BinaryReadOptions>,
): void {
): MessageShape<Desc> {
readMessage(
reflect(messageDesc, target),
new BinaryReader(bytes),
makeReadOptions(options),
false,
bytes.byteLength,
);
return target;
}

/**
Expand Down Expand Up @@ -154,21 +160,21 @@ export function readField(
);
break;
case "list":
readListField(message, reader, options, field, wireType);
readListField(reader, wireType, message.get(field), options);
break;
case "map":
readMapEntry(message, field, reader, options);
readMapEntry(reader, message.get(field), options);
break;
}
}

// Read a map field, expecting key field = 1, value field = 2
function readMapEntry(
message: ReflectMessage,
field: DescField & { fieldKind: "map" },
reader: BinaryReader,
map: ReflectMap,
options: BinaryReadOptions,
): void {
const field = map.field();
let key: MapEntryKey | undefined,
val: ScalarValue | ReflectMessage | undefined;
const end = reader.pos + reader.uint32();
Expand Down Expand Up @@ -209,21 +215,18 @@ function readMapEntry(
break;
}
}
message.setMapEntry(field, key, val);
map.set(key, val);
}

function readListField(
message: ReflectMessage,
reader: BinaryReader,
options: BinaryReadOptions,
field: DescField & { fieldKind: "list" },
wireType: WireType,
list: ReflectList,
options: BinaryReadOptions,
) {
const field = list.field();
if (field.listKind === "message") {
message.addListItem(
field,
readMessageField(reader, options, field) as never, // TODO: Investigate the type issue.
);
list.add(readMessageField(reader, options, field));
return;
}
const scalarType = field.scalar ?? ScalarType.INT32;
Expand All @@ -232,12 +235,12 @@ function readListField(
scalarType != ScalarType.STRING &&
scalarType != ScalarType.BYTES;
if (!packed) {
message.addListItem(field, readScalar(reader, scalarType));
list.add(readScalar(reader, scalarType));
return;
}
const e = reader.uint32() + reader.pos;
while (reader.pos < e) {
message.addListItem(field, readScalar(reader, scalarType));
list.add(readScalar(reader, scalarType));
}
}

Expand Down
Loading
Loading