-
Notifications
You must be signed in to change notification settings - Fork 349
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
feat: add options to schema output #437
Conversation
Hey @Vilsol , this is a great PR! Thanks for tackling this! Sorry for the delay in reviewing this, I'm still figuring out the details of extensions/unknown fields/etc. in protobuf, so took me awhile to find a dedicated time to look at the PR & think about thinks. I was thinking, for this change you need:
This basically sounds like adding unknown field support, i.e. while Wdyt about breaking that functionality out into its own PR? That we could we do:
We could potentially put unknown field support behind a feature flag, in case users want to turn it off, but it'd probably be a good thing to have on by default, I think... Other than that tactical approach, I had a few inline questions about maybe getting the options nudged into the existing metadata types where I think it might be a little more ergonomic to access, but otherwise this looks great! Wdyt about the approach of an initial/separate PR to support unknown fields, and then doing this PR on top of that one? |
The separate PR is not a bad idea, would mean that anything decoded would unconditionally get re-encoded guaranteeing consistency on any type. I will see if I can get something like that written up, but most likely only after the holidays. The reason why I did not decode the options in-place under the current |
Ah shoot, that's a good point; you're right that it's not clear how It could do a large destructure i.e.:
And then But for Maybe unknown fields should be set directly on the object, i.e. I'm a little wary to fundamentally change/restructure |
I tried using the branch for the simplest enum - I got this error with: FAILED!Cannot convert undefined or null to objectTypeError: Cannot convert undefined or null to object I tried to fix this with just adding a x || [] bypass there, but didn't get to see the EnumValue option manifest in the generated proto. I must be missing something. I just did clone, then yarn, then yarn build Lastly ran
A.proto:
|
@stephenh This should now be fully working. I have switched it over to the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vilsol apologies for the really late review; I had a few pending comments that I forgot to hit publish on, and then also have just been busy. I think this is good to merge as-is but I also had a few curious questions. Thanks!
src/schema.ts
Outdated
export function generateSchema(ctx: Context, fileDesc: FileDescriptorProto, sourceInfo: SourceInfo): Code[] { | ||
const { options } = ctx; | ||
const chunks: Code[] = []; | ||
|
||
fileDesc.extension.forEach((extension) => { | ||
extensionCache[extension.number] = extension; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the key to this cache probably needs to be extension.extendee
+extension.number
? I.e. the number could be used twice across different extensions.
chunks.push(code` | ||
type ProtoMetaMessageOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having this separate ProtoMetaMessageOptions
, could we output options within the existing fileDescriptor
structure?
I.e. a file-level option would be at:
protoMetadata.fileDescriptor.options["optionKey"]
I guess right now we're letting ts-poet .toJSON
-ify the descriptor
into the generated source code, and we'd probably need to nudge the options around a bit, i.e. like how you're base64 encoding messages...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why they get output outside of the existing object is because the existing objects get processed through fromPartial
which would strip all of those fields from the runtime.
They would also lose all type-safety if we did this.
src/schema.ts
Outdated
@@ -57,13 +96,181 @@ export function generateSchema(ctx: Context, fileDesc: FileDescriptorProto, sour | |||
descriptor.sourceCodeInfo?.location.filter((loc) => loc['leadingComments'] || loc['trailingComments']) || [], | |||
}; | |||
|
|||
let fileOptions: Code | undefined; | |||
if (descriptor.options) { | |||
fileOptions = encodedOptionsToOptions(ctx, (descriptor.options as any)['encodedOptions']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like here, I wonder if we could do something like:
if (descriptor.options) {
for (const key in Object.keys(descriptor.options.encodedOptions)) {
const ext = ...use extension cache to find `extensionee=FileDescriptor` && `number=key`
descriptor.options[ext.jsonName] = ...option value...
}
}
And then repeat for the message option / service options, etc...
integration/options/options-test.ts
Outdated
|
||
describe('options', () => { | ||
it('generates types correctly', () => { | ||
expect(protoMetadata).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This inline snapshot is huge; can we move it to a regular non-inline snapshot?
Also, could you add one or two tests that specifically load options? I get if the snapshot is correct, it will work, but I'm mostly want test coverage of the API you're specifically using to look up options, so that they act as documentation / reminder of what not to break, if we happen to otherwise refactor some aspects of the metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have extracted the snapshot to a separate file.
I don't fully understand what you mean by the extra tests you are requesting?
src/schema.ts
Outdated
return undefined; | ||
} | ||
const resultOptions: Code[] = []; | ||
for (const key of Object.keys(encodedOptions)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really minor could do:
for (const [key, value] of Object.entries(a)) {
src/schema.ts
Outdated
} | ||
} | ||
|
||
function encodedOptionsToOptions(ctx: Context, encodedOptions: { [key: number]: Uint8Array[] }): Code | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my own future future/maintenance, can we add like an in-source comment along the lines of:
/** Takes the protoc's input of options as proto-encoded messages, and turns them into embedded-able-in-source-code representations. */
@stephenh I have pushed some the requested changes and commented on the other review comments. |
@stephenh just wanted to notify you in case you missed it. The PR now complies with all the requested changes. |
Hey @Vilsol ! Thanks for the ping on this! I've looked at this PR a few times, but end up getting distracted because I'm just not that much of an expert on the options/schema output side of things. Everything looks great to my naive eyes, I just haven't had the chance to really think deeply about this; but that's fine, I'm just going to trust you on it. Thanks! :-) |
# [1.113.0](v1.112.2...v1.113.0) (2022-05-27) ### Features * add options to schema output ([#437](#437)) ([e8e4e39](e8e4e39))
🎉 This PR is included in version 1.113.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This adds options support for
outputSchema
. This would fix #295There is only one catch with this feature, and it's that it requires modification to the
ts-proto-descriptors
lib.For each of the following:
FileOptions
MessageOptions
FieldOptions
OneofOptions
EnumOptions
EnumValueOptions
ServiceOptions
MethodOptions
Inside of the
decode
method for each, before thewhile
loop it must first initialize a new untyped object:And then change the default
switch
case to the following:Due to these requirements, I have made this a draft PR.
I am not sure what would be the best approach to modify these files. Should we add a hardcoded check for these type names in this library itself, or should we make a git patch that gets applied every time before the
ts-proto-descirptors
lib gets published? Or maybe there is an even more elegant solution.I am also considering adding exact generated typings for the
ProtoMetadata
options, where the type foroptions
object would be a perfect match for the one in the actualprotoMetadata
object itself.Also, I had to resort to base64 encoding any messages and then they would get decoded at runtime, which is not a clean way to do this. Maybe there is some way to decode it at compile time?