(Typescript) Mark some metadata parameters as optional

[andrewmbenton]

There are a couple of method signatures in the generated TypeScript client that should mark the metadata parameter as optional, to reduce explicitly passing unnecessary null values:

grpc-web/javascript/net/grpc/web/generator/grpc_generator.cc

Lines 652 to 655 in 0ec55aa

	printer->Print(vars,
	"request: $input_type$,\n"
	"metadata: grpcWeb.Metadata | null): "
	"$promise$<$output_type$>;\n\n");

grpc-web/javascript/net/grpc/web/generator/grpc_generator.cc

Lines 670 to 674 in 0ec55aa

	printer->Print(vars,
	"request: $input_type$,\n"
	"metadata: grpcWeb.Metadata | null,\n"
	"callback?: (err: grpcWeb.RpcError,\n"
	" response: $output_type$) => void) {\n");

Making the metadata parameter optional in the method returning a Promise would bring the TypeScript client closer in line with the PromiseClient interface available when generating with import_style=commonjs+dts.

Resolves #1368

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: andrewmbenton / name: Andrew Benton (95b74ac, d8d3541)

[sampajano]

@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! :)

[sampajano]

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!

[andrewmbenton]

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.

[sampajano]

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]

Thanks so much for the change again! Much appreciated!