(Typescript) Mark some metadata parameters as optional#1369
(Typescript) Mark some metadata parameters as optional#1369sampajano merged 2 commits intogrpc:masterfrom
metadata parameters as optional#1369Conversation
sampajano
left a comment
sampajano
left a comment
There was a problem hiding this comment.
@andrewmbenton Thanks so much for this change (and sorry for the delay)! This makes a lot of sense! 😃
Just one comment! Will merge once it's addressed/acked, and when tests passes!
Thanks again for the contribution! :)
| printer->Print(vars, | ||
| "request: $input_type$,\n" | ||
| "metadata: grpcWeb.Metadata | null,\n" | ||
| "metadata?: grpcWeb.Metadata | null,\n" |
There was a problem hiding this comment.
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
PromiseClientinterface available when generating withimport_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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
sampajano
left a comment
sampajano
left a comment
There was a problem hiding this comment.
Thanks so much for the change again! Much appreciated!
sampajano
changed the title
metadata parameters as optionalmetadata parameters as optional
3 participants
There are a couple of method signatures in the generated TypeScript client that should mark the
metadataparameter as optional, to reduce explicitly passing unnecessarynullvalues:grpc-web/javascript/net/grpc/web/generator/grpc_generator.cc
Lines 652 to 655 in 0ec55aa
grpc-web/javascript/net/grpc/web/generator/grpc_generator.cc
Lines 670 to 674 in 0ec55aa
Making the
metadataparameter optional in the method returning aPromisewould bring the TypeScript client closer in line with thePromiseClientinterface available when generating withimport_style=commonjs+dts.Resolves #1368