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.

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: Strip only last .proto suffix for DescFile.name #807

Merged
merged 2 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion packages/protobuf-bench/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ server would usually do.

| code generator | bundle size | minified | compressed |
|---------------------|------------------------:|-----------------------:|-------------------:|
| protobuf-es | 127,064 b | 65,515 b | 15,970 b |
| protobuf-es | 127,054 b | 65,501 b | 15,983 b |
| protobuf-javascript | 394,384 b | 288,654 b | 45,122 b |
6 changes: 3 additions & 3 deletions packages/protobuf-test/src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ export async function compileFileDescriptorSet(
return fromBinary(FileDescriptorSetDesc, bytes);
}

export async function compileFile(proto: string) {
export async function compileFile(proto: string, name = "input.proto") {
upstreamProtobuf = upstreamProtobuf ?? new UpstreamProtobuf();
const bytes = await upstreamProtobuf.compileToDescriptorSet(
{
"input.proto": proto,
[name]: proto,
},
{
includeImports: true,
Expand All @@ -49,7 +49,7 @@ export async function compileFile(proto: string) {
);
const fds = fromBinary(FileDescriptorSetDesc, bytes);
const reg = createFileRegistry(fds);
const file = reg.getFile("input.proto");
const file = reg.getFile(name);
assert(file);
return file;
}
Expand Down
41 changes: 20 additions & 21 deletions packages/protobuf-test/src/reflect/registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
import {
compileEnum,
compileField,
compileFile,
compileFileDescriptorSet,
compileMessage,
compileMethod,
Expand Down Expand Up @@ -502,31 +503,16 @@ describe("createFileRegistry()", function () {

describe("DescFile", () => {
test("proto2 syntax", async () => {
const fileDescriptorSet = await compileFileDescriptorSet({
"a.proto": `syntax="proto2";`,
});
const reg = createFileRegistry(fileDescriptorSet);
const descFile = reg.getFile("a.proto");
expect(descFile).toBeDefined();
expect(descFile?.edition).toBe(Edition.EDITION_PROTO2);
const file = await compileFile(`syntax="proto2";`);
expect(file.edition).toBe(Edition.EDITION_PROTO2);
});
test("proto3 syntax", async () => {
const fileDescriptorSet = await compileFileDescriptorSet({
"a.proto": `syntax="proto3";`,
});
const reg = createFileRegistry(fileDescriptorSet);
const descFile = reg.getFile("a.proto");
expect(descFile).toBeDefined();
expect(descFile?.edition).toBe(Edition.EDITION_PROTO3);
const file = await compileFile(`syntax="proto3";`);
expect(file.edition).toBe(Edition.EDITION_PROTO3);
});
test("edition 2023", async () => {
const fileDescriptorSet = await compileFileDescriptorSet({
"a.proto": `edition = "2023";`,
});
const reg = createFileRegistry(fileDescriptorSet);
const descFile = reg.getFile("a.proto");
expect(descFile).toBeDefined();
expect(descFile?.edition).toBe(Edition.EDITION_2023);
const file = await compileFile(`edition = "2023";`);
expect(file.edition).toBe(Edition.EDITION_2023);
});
test("dependencies", async () => {
const fileDescriptorSet = await compileFileDescriptorSet({
Expand All @@ -542,6 +528,19 @@ describe("DescFile", () => {
expect(a?.dependencies.length).toBe(2);
expect(a?.dependencies.map((f) => f.name)).toStrictEqual(["b", "c"]);
});
describe("name", () => {
test("is proto file name without .proto suffix", async () => {
const file = await compileFile(`syntax="proto3";`, "foo/bar/baz.proto");
expect(file.name).toBe("foo/bar/baz");
});
test("strips only last .proto", async () => {
const file = await compileFile(
`syntax="proto3";`,
"foo.proto/baz.proto.proto",
);
expect(file.name).toBe("foo.proto/baz.proto");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the change above, name would be "foo/baz.proto.proto".

});
});
});

describe("DescEnum", () => {
Expand Down
6 changes: 3 additions & 3 deletions packages/protobuf/src/reflect/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -443,15 +443,15 @@ function addFile(proto: FileDescriptorProto, reg: MutableRegistry): void {
proto,
deprecated: proto.options?.deprecated ?? false,
edition: getFileEdition(proto),
name: proto.name.replace(/\.proto/, ""),
name: proto.name.replace(/\.proto$/, ""),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, this removed the first sub-string.

dependencies: findFileDependencies(proto, reg),
enums: [],
messages: [],
extensions: [],
services: [],
toString(): string {
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- we asserted above
return `file ${this.proto.name}`;
return `file ${proto.name}`;
},
};
const mapEntriesStore = new Map<string, DescMessage>();
Expand Down Expand Up @@ -829,7 +829,7 @@ function newField(
keyField.scalar != ScalarType.FLOAT &&
keyField.scalar != ScalarType.DOUBLE,
);
const valueField = mapEntry.fields.find((f) => f.proto.number === 2);
const valueField = mapEntry.fields.find((f) => f.number === 2);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use DescField.number here, no need to go through FieldDescriptorProto.

assert(valueField);
assert(valueField.fieldKind != "list" && valueField.fieldKind != "map");
field.mapKey = keyField.scalar;
Expand Down
Loading