947 - improved Typescript oneof generated types #1518
Conversation
|
@Vuhag123 Hi Vuha, real quick: Could you include some more details in the PR description? with examples showcasing what is being changed? Thanks! 😃 |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve TypeScript type definitions for oneof fields in gRPC-web by introducing discriminated union types for the AsObject representation. The changes refactor the PrintProtoDtsMessage function to generate more type-safe TypeScript definitions.
Key changes:
- Refactored code generator with improved comments and organization
- Changed AsObject type generation to use discriminated unions with
oneofKinddiscriminant for messages containing oneofs - Used consistent
std::stringtypes instead ofstring
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (int oi = 0; oi < desc->oneof_decl_count(); oi++) { | ||
| const OneofDescriptor* oneof = desc->oneof_decl(oi); |
There was a problem hiding this comment.
Inconsistent use of oneof_decl_count() vs real_oneof_decl_count(). This loop iterates over all oneof declarations (including synthetic ones from proto3 optional fields), but the case methods are only generated for real oneofs (line 911). This will cause the AsObject type to include discriminated union syntax for synthetic oneofs (proto3 optional fields) that don't have corresponding case enums, leading to TypeScript type errors.
Consider using real_oneof_decl_count() and real_oneof_decl(oi) instead to only apply discriminated union syntax to real oneofs.
| for (int oi = 0; oi < desc->oneof_decl_count(); oi++) { | |
| const OneofDescriptor* oneof = desc->oneof_decl(oi); | |
| for (int oi = 0; oi < desc->real_oneof_decl_count(); oi++) { | |
| const OneofDescriptor* oneof = desc->real_oneof_decl(oi); |
| printer->Print("export type AsObject = (\n"); | ||
| printer->Indent(); | ||
|
|
||
| for (int oi = 0; oi < desc->oneof_decl_count(); oi++) { | ||
| const OneofDescriptor* oneof = desc->oneof_decl(oi); | ||
| for (int fi = 0; fi < oneof->field_count(); fi++) { | ||
| const FieldDescriptor* field = oneof->field(fi); | ||
| std::string js_field_name = CamelCaseJSFieldName(field); | ||
| if (IsReserved(js_field_name)) js_field_name = "pb_" + js_field_name; | ||
| vars["js_field_name"] = js_field_name; | ||
| vars["js_field_type"] = AsObjectFieldType(field, file); | ||
|
|
||
| printer->Print(vars, " | { oneofKind: \"$js_field_name$\"; $js_field_name$: $js_field_type$ }\n"); | ||
| } | ||
| } | ||
|
|
||
| vars["js_field_name"] = js_field_name; | ||
| vars["js_field_type"] = AsObjectFieldType(field, file); | ||
| if (!field->has_presence()) { | ||
| printer->Print(vars, "$js_field_name$: $js_field_type$;\n"); | ||
| } else { | ||
| printer->Print(vars, "$js_field_name$?: $js_field_type$;\n"); | ||
| printer->Print(" | { oneofKind: undefined }\n"); | ||
| printer->Outdent(); | ||
| printer->Print(") & {\n"); |
There was a problem hiding this comment.
The discriminated union design is semantically incorrect for messages with multiple oneofs. The current implementation uses a single oneofKind discriminant for all fields across all oneofs, which means the type only allows one field to be set at a time across ALL oneofs. However, protobuf semantics allow one field per oneof to be set simultaneously.
For example, with two oneofs first and second, the generated type would incorrectly prevent setting a field from first and a field from second at the same time.
The type should instead use separate discriminants for each oneof (e.g., firstKind and secondKind) or use a different approach like nested objects for each oneof group.
| if (IsReserved(js_field_name)) js_field_name = "pb_" + js_field_name; | ||
| vars["js_field_name"] = js_field_name; | ||
| vars["js_field_type"] = AsObjectFieldType(field, file); | ||
| // Preserve previous presence semantics: use optional for fields with presence or repeated/map? keep simple optional |
There was a problem hiding this comment.
The comment is unclear and appears incomplete. The phrase "or repeated/map? keep simple optional" contains a question mark and reads like an unfinished thought or note to self rather than a proper code comment.
Consider clarifying this to something like: "Preserve previous presence semantics: all non-oneof fields are optional"
| // Preserve previous presence semantics: use optional for fields with presence or repeated/map? keep simple optional | |
| // Preserve previous presence semantics: all non-oneof fields are optional |
| if (desc->oneof_decl_count() > 0) { | ||
| // Type-safe union for oneofs (string discriminant = field name) | ||
| printer->Print("export type AsObject = (\n"); | ||
| printer->Indent(); | ||
|
|
||
| for (int oi = 0; oi < desc->oneof_decl_count(); oi++) { | ||
| const OneofDescriptor* oneof = desc->oneof_decl(oi); |
There was a problem hiding this comment.
Inconsistent use of oneof_decl_count() vs real_oneof_decl_count(). The code uses real_oneof_decl_count() on line 911 for generating case methods, but uses oneof_decl_count() here for determining whether to use the discriminated union approach. This could cause a mismatch where the AsObject uses discriminated union syntax for synthetic oneofs (proto3 optional fields) that don't have corresponding case enums, leading to TypeScript compilation errors.
Consider using real_oneof_decl_count() instead of oneof_decl_count() to only apply discriminated union syntax to real oneofs.
| if (desc->oneof_decl_count() > 0) { | |
| // Type-safe union for oneofs (string discriminant = field name) | |
| printer->Print("export type AsObject = (\n"); | |
| printer->Indent(); | |
| for (int oi = 0; oi < desc->oneof_decl_count(); oi++) { | |
| const OneofDescriptor* oneof = desc->oneof_decl(oi); | |
| if (desc->real_oneof_decl_count() > 0) { | |
| // Type-safe union for real oneofs (string discriminant = field name) | |
| printer->Print("export type AsObject = (\n"); | |
| printer->Indent(); | |
| for (int oi = 0; oi < desc->real_oneof_decl_count(); oi++) { | |
| const OneofDescriptor* oneof = desc->real_oneof_decl(oi); |
2 participants
Fix (#947) - Improved Typescript oneof generated types.