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: Support option retention #827

Merged
merged 2 commits into from
May 2, 2024
Merged

V2: Support option retention #827

merged 2 commits into from
May 2, 2024

Conversation

timostamm
Copy link
Member

This PR adds support for option retention: Options can specify whether they should be included in generated code (making them accessible at runtime), or be only available for plugins, while generating code.

With this change, options that specify retention = RETENTION_SOURCE are available in plugins written on top of @bufbuild/protoplugin. But they will not be included in the code generated by protoc-gen-es.

Comment on lines +219 to +228
// Our goal is to provide options with source retention to plugin authors.
// CodeGeneratorRequest.proto_file elides options with source retention for
// files to generate. For these files, we take the file from source_file_descriptors,
// which does include options with source retention.
const allProtoWithSourceOptions = request.protoFile.map((protoFile) => {
const sourceFile = request.sourceFileDescriptors.find(
(s) => s.name == protoFile.name,
);
return sourceFile ?? protoFile;
});
Copy link
Member Author

Choose a reason for hiding this comment

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

This is backwards compatible. Old compilers will not populate source_file_descriptors, and we keep using proto_file.

Comment on lines 266 to 269
function getFileDescCall(f: GeneratedFile, file: DescFile, schema: Schema) {
const sourceFile = file.proto;
const runtimeFile = schema.proto.protoFile.find(f => f.name == sourceFile.name);
const info = embedFileDesc(runtimeFile ?? sourceFile);
Copy link
Member Author

Choose a reason for hiding this comment

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

This removes source retention options from embedded file descriptors generated by protoc-gen-es. We simply do the reverse of packages/protoplugin/src/ecmascript/schema.ts:223.

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth a comment in here about why we want the version in schema.proto.protoFile instead of the provided file.proto, because it would already have omitted source-only options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, added in 2ca5ebd

@timostamm timostamm marked this pull request as ready for review May 1, 2024 16:56
Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 266 to 269
function getFileDescCall(f: GeneratedFile, file: DescFile, schema: Schema) {
const sourceFile = file.proto;
const runtimeFile = schema.proto.protoFile.find(f => f.name == sourceFile.name);
const info = embedFileDesc(runtimeFile ?? sourceFile);
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth a comment in here about why we want the version in schema.proto.protoFile instead of the provided file.proto, because it would already have omitted source-only options.

@timostamm timostamm merged commit b81a538 into v2 May 2, 2024
6 checks passed
@timostamm timostamm deleted the tstamm/option-retention branch May 2, 2024 07:34
@timostamm timostamm mentioned this pull request May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants