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.

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

(Typescript) Mark some metadata parameters as optional #1369

Merged
merged 2 commits into from
Oct 16, 2023
Merged

(Typescript) Mark some metadata parameters as optional #1369

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95b74ac
feat(typescript): mark some `metadata` parameters as optional
andrewmbenton Oct 5, 2023
d8d3541
Merge branch 'master' into patch-1
andrewmbenton Oct 13, 2023
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
4 changes: 2 additions & 2 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 @@ -653,7 +653,7 @@ void PrintTypescriptFile(Printer* printer, const FileDescriptor* file,
printer->Indent();
printer->Print(vars,
"request: $input_type$,\n"
"metadata: grpcWeb.Metadata | null): "
"metadata?: grpcWeb.Metadata | null): "
"$promise$<$output_type$>;\n\n");
printer->Outdent();

Expand All @@ -671,7 +671,7 @@ void PrintTypescriptFile(Printer* printer, const FileDescriptor* file,
printer->Indent();
printer->Print(vars,
"request: $input_type$,\n"
"metadata: grpcWeb.Metadata | null,\n"
"metadata?: grpcWeb.Metadata | null,\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You've mentioned this in the associated issue (which i copied to this PR too :))

Making these parameters optional I believe would bring the TypeScript client closer in line with the PromiseClient interface available when generating with import_style=commonjs+dts.

I believe this is true for the other change! But not this one.

It seems that callback? has only 1 occurrence the whole file, and is a unique feature in TS codegen :)

I don't mind this change also, but maybe you could consider rewording your comment (and PR description now :)) to note that it only applies to the other change? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will reword my comments. You are right that the PromiseClient generated with import_style=commonjs+dts doesn't seem to have any generated methods with a callback parameter.

For true parity with the PromiseClient I would remove the null from the metadata param's type, but that would break user code that passes null so I didn't propose that change.

I am also happy to revert the change to the function that doesn't return a Promise if that's preferable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For true parity with the PromiseClient I would remove the null from the metadata param's type, but that would break user code that passes null so I didn't propose that change.

Definitely! Appreciate the caution and i'd agree!

I am also happy to revert the change to the function that doesn't return a Promise if that's preferable.

Yeah i think that's an option but i don't see this making things more inconsistent than they already were (at least my glimpse of it) so i think this is still a good thing to keep 😃

Thanks for offering tho!

"callback?: (err: grpcWeb.RpcError,\n"
" response: $output_type$) => void) {\n");
printer->Print(vars, "if (callback !== undefined) {\n");
Expand Down