Skip to content

947 - improved Typescript oneof generated types #1518

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

Open
Vuhag123 wants to merge 1 commit into
base: master
Choose a base branch
from
+112 −68
Open

947 - improved Typescript oneof generated types #1518

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
180 changes: 112 additions & 68 deletions javascript/net/grpc/web/generator/grpc_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,116 +833,160 @@ void PrintProtoDtsOneofCase(Printer* printer, const OneofDescriptor* desc) {
printer->Print("}\n");
}

void PrintProtoDtsMessage(Printer* printer, const Descriptor* desc,
const FileDescriptor* file) {
const string& class_name = desc->name();
std::map<string, string> vars;
void PrintProtoDtsMessage(Printer* printer, const Descriptor* desc, const FileDescriptor* file) {
const std::string& class_name = desc->name();
std::map<std::string, std::string> vars;
vars["class_name"] = class_name;

// Print class declaration (methods will be declared in the class scope)
printer->Print(vars, "export class $class_name$ extends jspb.Message {\n");
printer->Indent();

// --- Generate getters/setters/has/clear/add for each field ---
for (int i = 0; i < desc->field_count(); i++) {
const FieldDescriptor* field = desc->field(i);

vars["js_field_name"] = SafeAccessorName(JSFieldName(field));
// Use SafeAccessorName on JSFieldName so reserved names get the $ suffix.
std::string safe_name = SafeAccessorName(JSFieldName(field));
vars["js_field_name"] = safe_name;
vars["js_field_type"] = JSFieldType(field, file);
if (field->type() != FieldDescriptor::TYPE_MESSAGE ||
field->is_repeated()) {
printer->Print(vars, "get$js_field_name$(): $js_field_type$;\n");

// getX(): type or type | undefined for singular message presence
if (field->type() != FieldDescriptor::TYPE_MESSAGE || field->is_repeated()) {
printer->Print(vars, " get$js_field_name$(): $js_field_type$;\n");
} else {
printer->Print(vars,
"get$js_field_name$(): $js_field_type$ | undefined;\n");
printer->Print(vars, " get$js_field_name$(): $js_field_type$ | undefined;\n");
}

// bytes helpers
if (field->type() == FieldDescriptor::TYPE_BYTES && !field->is_repeated()) {
printer->Print(vars,
"get$js_field_name$_asU8(): Uint8Array;\n"
"get$js_field_name$_asB64(): string;\n");
// two additional accessors for bytes fields
printer->Print(vars, " get$js_field_name$_asU8(): Uint8Array;\n");
printer->Print(vars, " get$js_field_name$_asB64(): string;\n");
}
if (!field->is_map() && (field->type() != FieldDescriptor::TYPE_MESSAGE ||
field->is_repeated())) {
printer->Print(vars,
"set$js_field_name$(value: $js_field_type$): "
"$class_name$;\n");

// setX(...)
if (!field->is_map() && (field->type() != FieldDescriptor::TYPE_MESSAGE || field->is_repeated())) {
printer->Print(vars, " set$js_field_name$(value: $js_field_type$): $class_name$;\n");
} else if (!field->is_map()) {
printer->Print(vars,
"set$js_field_name$(value?: $js_field_type$): "
"$class_name$;\n");
// message singular
printer->Print(vars, " set$js_field_name$(value?: $js_field_type$): $class_name$;\n");
}

// hasX()
if (field->has_presence()) {
printer->Print(vars, "has$js_field_name$(): boolean;\n");
printer->Print(vars, " has$js_field_name$(): boolean;\n");
}
if (field->type() == FieldDescriptor::TYPE_MESSAGE ||
field->has_presence() || field->is_repeated() || field->is_map()) {
printer->Print(vars, "clear$js_field_name$(): $class_name$;\n");

// clearX()
if (field->type() == FieldDescriptor::TYPE_MESSAGE || field->has_presence() || field->is_repeated() || field->is_map()) {
printer->Print(vars, " clear$js_field_name$(): $class_name$;\n");
}

// repeated addX()
if (field->is_repeated() && !field->is_map()) {
// element name/type for add
std::string elem_name = SafeAccessorName(JSElementName(field));
std::string elem_type = JSElementType(field, file);
vars["js_field_name"] = elem_name;
vars["js_field_type"] = elem_type;

vars["js_field_name"] = SafeAccessorName(JSElementName(field));
vars["js_field_type"] = JSElementType(field, file);
if (field->type() != FieldDescriptor::TYPE_MESSAGE) {
printer->Print(vars,
"add$js_field_name$(value: $js_field_type$, "
"index?: number): $class_name$;\n");
// add(value: T, index?: number): Message
printer->Print(vars, " add$js_field_name$(value: $js_field_type$, index?: number): $class_name$;\n");
} else {
printer->Print(vars,
"add$js_field_name$(value?: $js_field_type$, "
"index?: number): $js_field_type$;\n");
// add(value?: T, index?: number): T
printer->Print(vars, " add$js_field_name$(value?: $js_field_type$, index?: number): $js_field_type$;\n");
}

// restore js_field_name for subsequent iterations if needed
vars["js_field_name"] = SafeAccessorName(JSFieldName(field));
vars["js_field_type"] = JSFieldType(field, file);
}

printer->Print("\n");
}

// --- Generate real oneof get<Oneof>Case() methods ---
for (int i = 0; i < desc->real_oneof_decl_count(); i++) {
const OneofDescriptor *oneof = desc->real_oneof_decl(i);
vars["js_oneof_name"] = ToUpperCamel(ParseLowerUnderscore(oneof->name()));
printer->Print(
vars, "get$js_oneof_name$Case(): $class_name$.$js_oneof_name$Case;\n");
const OneofDescriptor* oneof = desc->real_oneof_decl(i);
std::string js_oneof_name = ToUpperCamel(ParseLowerUnderscore(oneof->name()));
vars["js_oneof_name"] = js_oneof_name;
printer->Print(vars, " get$js_oneof_name$Case(): $class_name$.$js_oneof_name$Case;\n");
printer->Print("\n");
}

printer->Print(
vars,
"serializeBinary(): Uint8Array;\n"
"toObject(includeInstance?: boolean): "
"$class_name$.AsObject;\n"
"static toObject(includeInstance: boolean, msg: $class_name$): "
"$class_name$.AsObject;\n"
"static serializeBinaryToWriter(message: $class_name$, writer: "
"jspb.BinaryWriter): void;\n"
"static deserializeBinary(bytes: Uint8Array): $class_name$;\n"
"static deserializeBinaryFromReader(message: $class_name$, reader: "
"jspb.BinaryReader): $class_name$;\n");
printer->Print(vars,
" serializeBinary(): Uint8Array;\n"
" toObject(includeInstance?: boolean): $class_name$.AsObject;\n"
" static toObject(includeInstance: boolean, msg: $class_name$): $class_name$.AsObject;\n"
" static serializeBinaryToWriter(message: $class_name$, writer: jspb.BinaryWriter): void;\n"
" static deserializeBinary(bytes: Uint8Array): $class_name$;\n"
" static deserializeBinaryFromReader(message: $class_name$, reader: jspb.BinaryReader): $class_name$;\n");
printer->Outdent();
printer->Print("}\n\n");

// --- Namespace with AsObject and nested types/enums/oneof-case enums ---
printer->Print(vars, "export namespace $class_name$ {\n");
printer->Indent();
printer->Print("export type AsObject = {\n");
printer->Indent();
for (int i = 0; i < desc->field_count(); i++) {
const FieldDescriptor* field = desc->field(i);

string js_field_name = CamelCaseJSFieldName(field);
if (IsReserved(js_field_name)) {
js_field_name = "pb_" + js_field_name;

// Generate AsObject:
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);
Comment on lines +939 to +940
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.
Comment on lines +934 to +940
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.
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");
Comment on lines +936 to +954
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
printer->Indent();

// Add non-oneof fields to the intersection object
for (int i = 0; i < desc->field_count(); i++) {
const FieldDescriptor* field = desc->field(i);
if (!field->containing_oneof()) {
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);
// Preserve previous presence semantics: use optional for fields with presence or repeated/map? keep simple optional
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

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

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"

Suggested change
// 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

Copilot uses AI. Check for mistakes.
printer->Print(vars, " $js_field_name$?: $js_field_type$;\n");
}
}

printer->Outdent();
printer->Print("};\n");
} else {
// No oneofs: legacy object
printer->Print("export type AsObject = {\n");
printer->Indent();
for (int i = 0; i < desc->field_count(); i++) {
const FieldDescriptor* field = desc->field(i);
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, " $js_field_name$?: $js_field_type$;\n");
}
printer->Outdent();
printer->Print("};\n");
}
printer->Outdent();
printer->Print("};\n");

for (int i = 0; i < desc->nested_type_count(); i++) {
if (desc->nested_type(i)->options().map_entry()) {
continue;
}
if (desc->nested_type(i)->options().map_entry()) continue;
printer->Print("\n");
PrintProtoDtsMessage(printer, desc->nested_type(i), file);
}
Expand Down