MethodDescriptor<REQ, RESP>.getName() is not a function

[huyungtang]

version: 1.4.0

When calling Request<any, any>.getMethodDescriptor().getName() in unaryInterceptors, all functions of MethodDescriptor<REQ, RESP> disappeared! In V1.3.1 works fine.

[sampajano]

@huyungtang thanks for the report!

Unfortunately that's expected due to the rollback here:
#1199

There's a internal code size increase due to exposing those methods and hence we're leaving them out for now..

If there's a big demand for this we could revisit this decision (by introducing a fork maybe but we generally want to minimize internal & Github divergence).

Thanks again and sorry for inconveniences!

---

CC'ing @stanley-cheung too In case you have any ideas on the general policy here. thanks!

[huyungtang]

@sampajano, @stanley-cheung thanks!

But if the definition could be removed at the same time, that would give us lot of help to find out the changes not just in runtime.

[sampajano]

@sampajano, @stanley-cheung thanks!

But if the definition could be removed at the same time, that would give us lot of help to find out the changes not just in runtime.

That's actually a very good point..

@stanley-cheung Do you think that for now, we can remove MethodDescriptor APIs from Typescript interface?

I'm asking since you've made a comment earlier when we first made the fix here: #1160 (review)

[pro-wh]

remove MethodDescriptor APIs from Typescript interface?

it's used in the AbstractClientBase methods though, thenableCall et al.

[sampajano]

remove MethodDescriptor APIs from Typescript interface?

it's used in the AbstractClientBase methods though, thenableCall et al.

It is indeed used.. But i guess what's being considered here is that whether we can remove the method definitions of MethodDescriptor (the class itself can be kept if needed)?

And i guess that generally hinges on whether we would support custom MethodDescriptors (like in your use case).. :)

[pro-wh]

remove the method definitions of MethodDescriptor (the class itself can be kept if needed)

I think that's fine with us if you get rid of the types and retain the class. We'll use @ts-ignore as needed, while we export our own typed methods to the users of our library.

custom MethodDescriptors (like in your use case)

So far there seems to be no conflict here either. We use the same requestSerializeFn and responseDeserializeFn constructor parameters that the generated grpc code uses.

If .name breaks in the future, I suppose we can mimic what the generated grpc code does and repeat the method name in the thenableCall instead of accessing methodDescriptor.name.

[sampajano]

remove the method definitions of MethodDescriptor (the class itself can be kept if needed)

I think that's fine with us if you get rid of the types and retain the class. We'll use @ts-ignore as needed, while we export our own typed methods to the users of our library.

custom MethodDescriptors (like in your use case)

So far there seems to be no conflict here either. We use the same requestSerializeFn and responseDeserializeFn constructor parameters that the generated grpc code uses.

If .name breaks in the future, I suppose we can mimic what the generated grpc code does and repeat the method name in the thenableCall instead of accessing methodDescriptor.name.

oh wow. thanks so much for being understanding and flexible about the approaches..

I guess if you go to this length to use our library, we have no reason not to try to make it work for your use case. Given that, we both agree that this remains an unofficial / non-promoted way to do it. :)

Although for my curiosity, i'm wondering what's the main benefit you get from using our library — given that AFAIU Protobuf and Codegen are the biggest reason why this library is needed and you're not using them.. 😅

[pro-wh]

it's really this stuff that goes around that, e.g. how do you set the right headers, how do you decode the grpc framing bytes https://github.com/grpc/grpc-web/blob/1.4.0/javascript/net/grpc/web/grpcwebclientbase.js#L291, how do you incrementally parse a streaming response https://github.com/grpc/grpc-web/blob/1.4.0/javascript/net/grpc/web/grpcwebclientreadablestream.js#L135, these sort of things. I don't doubt it's smaller than the whole protobuf and codegen systems, but it's still hundreds of lines of code that we're able to use

[sampajano]

it's really this stuff that goes around that, e.g. how do you set the right headers, how do you decode the grpc framing bytes https://github.com/grpc/grpc-web/blob/1.4.0/javascript/net/grpc/web/grpcwebclientbase.js#L291, how do you incrementally parse a streaming response https://github.com/grpc/grpc-web/blob/1.4.0/javascript/net/grpc/web/grpcwebclientreadablestream.js#L135, these sort of things. I don't doubt it's smaller than the whole protobuf and codegen systems, but it's still hundreds of lines of code that we're able to use

Aha ok. That's good to know. Thanks for explaining :)

---

So i guess - i'll go ahead and remove the MethodDescriptor methods from our Typescript type definitions (as it's not really working as of 1.4.0).

@pro-wh I'll work with you to get your PR in after i cut a quick bug fix release (1.4.1) today. And i'll do a follow up release (1.4.2) after removing the methods here (and when your PR is in). Thanks :)

[pro-wh]

imo keep the MethodDescriptor class and constructor in the typescript declarations, as they'll still exist, and closure will use them, and generated grpc code will use them (and our library will use them 🤭). remove only the getters to match the removal of those from the exports. nvm that's consistent with your proposal

would it be okay to add .name field to the typescript declaration? i.e. does closure know not to rename it, or was it by some inaccuracy that it's not obfuscated in the dist files?

[sampajano]

(sorry was out last 2 days just returned :))

imo keep the MethodDescriptor class and constructor in the typescript declarations, as they'll still exist, and closure will use them, and generated grpc code will use them (and our library will use them 🤭). remove only the getters to match the removal of those from the exports. nvm that's consistent with your proposal

aha right sounds good! Yeahh i wasn't actually thinking much about the constructor but makes sense to keep it :)

would it be okay to add .name field to the typescript declaration? i.e. does closure know not to rename it, or was it by some inaccuracy that it's not obfuscated in the dist files?

I actually have no idea why closure doesn't obfuscate it (and a bit surprised that it wasn't 😅).. For example, i couldn't find other field names like requestSerializeFn in the generated index.js file.. 😃

In any case, whether we add it to the typescript declaration shouldn't have an effect on whether Closure decides to rename it.. and also in this case i don't think it's really appropriate to expose the .name property on the typescript definition. (If it really comes down to this, i can see if we can just expose the getName() method for your use case.. :))

[pro-wh]

Exposing getName() would be good. Fingers crossed on it not affecting your code size.

[sampajano]

Exposing getName() would be good. Fingers crossed on it not affecting your code size.

aha ok.. yeahh i doubt it would be a big deal (i suspect the earlier code size increase was due to the other methods which cause a chain of types to be included). Fingers crossed :)

I'll make a PR to clean up the index.d.ts definitions and expose the getName() method. And will sync it and allow it to sit for a week to see if it's raising any alarm. If not, i'll cut a new release with your fixes too :)

UPDATE: Done.. Will circle back in ~1 week time :)