search

grpc / grpc-web

gRPC for Web Clients
https://grpc.io
Apache License 2.0
8.51k stars 766 forks source link

GrpcWebClientReadableStream: keep falsy data #1230

Closed pro-wh closed 1 year ago

pro-wh commented 2 years ago

cross reference https://github.com/grpc/grpc-web/pull/1025

When using a different codec where primitive values are allowed (we use CBOR), the current implementation omits calling the data listeners on falsy values, such as 0 and null. But we'd like to have them.

Is there anything else that relies on the current behavior that omits these values?

Discussion of changes in GrpcWebClientBase:

Current implementation uses setCallback_ in a special "useUnaryResponse" mode. In this mode, additional calls are made to the callback with status and metadata parameters, then a final time with both error and response set to null.

In order to support falsy and even null message, this changes the final callback call to pass true to a new explicit trigger parameter. callback(null, null) now means "no error, and the response was null" (and similar for other falsy responses).

setCallback_ is private, and the only public wrapper does not allow the "useUnaryResponse" mode, so it should be safe to make this change.

pro-wh commented 2 years ago

oh oops this should skip if deserializing failed. I'll add some logic to do that

added

pro-wh commented 2 years ago

there are more changes needed in thenableCall

sampajano commented 2 years ago

Hi! Thanks for the contribution and sorry for the delay!

2 quick questions:

  1. Curious how are you using a custom codec? I'm still somewhat new and didn't know that's possible 😃
  2. Would it be possible for you to showcase your new use case by adding a test?

thanks!

pro-wh commented 2 years ago

Hi, thanks for helping out with this.

How to use a custom codec:

sample code https://github.com/oasisprotocol/oasis-sdk/blob/9cfbb9246ba1797e475728a48d580b0eb8a352d3/client-sdk/ts-web/core/src/client.ts#L22

We haven't been using .proto files to define the methods, so we construct a bunch of our own MethodDescriptor objects. Thus all it took for us was to pass the https://github.com/grpc/grpc-web/blob/1.3.1/javascript/net/grpc/web/methoddescriptor.js#L33 responseDeserializerFn

Adding a test

Ya let me try adding one

sampajano commented 2 years ago

How to use a custom codec:

sample code https://github.com/oasisprotocol/oasis-sdk/blob/9cfbb9246ba1797e475728a48d580b0eb8a352d3/client-sdk/ts-web/core/src/client.ts#L22

We haven't been using .proto files to define the methods, so we construct a bunch of our own MethodDescriptor objects. Thus all it took for us was to pass the https://github.com/grpc/grpc-web/blob/1.3.1/javascript/net/grpc/web/methoddescriptor.js#L33 responseDeserializerFn

Oh aha! Intersting! Thanks for the info! I guess that's one way of using the library (but without the codegen) 😃

I wonder if we mention the use of library in this way on any documentation? Or this is just one creative way you found out about using it? (I'm guess in the latter case I'm not sure if such use will be "officially supported" going forward, even if we could probably fix a few non-harming edge-cases right now) 😃

Adding a test

Ya let me try adding one

thanks!!

pro-wh commented 2 years ago
sampajano commented 2 years ago

Ah thanks for the info!

I did a quick search and found that the majority of mentioning for custom codec is for golang. (there are some mentions on other languages but nothing seems officially documented.) So personally i'd still consider this a borderline edge use case :) Happy to take small patches if it's easy to maintain, but probably no commitment for major feature support going forward :)

pro-wh commented 1 year ago

aaah I just saw the new release and realized this was still pending

So personally i'd still consider this a borderline edge use case :) Happy to take small patches if it's easy to maintain, but probably no commitment for major feature support going forward :)

we're happy with that too. if you could please review :pray:

sampajano commented 1 year ago

aaah I just saw the new release and realized this was still pending

aha right i've forgot as well :)

So personally i'd still consider this a borderline edge use case :) Happy to take small patches if it's easy to maintain, but probably no commitment for major feature support going forward :)

we're happy with that too. if you could please review 🙏

thanks for your interest here! On a 2nd thought, I'll need to check with the team on our stance on custom codec before getting back at you. Thanks!

pro-wh commented 1 year ago

Sounds good, thanks for checking with the team

sampajano commented 1 year ago

Of course!

Oh and actually, since you'll be depending on MethodDescriptor, i guess this will be related to https://github.com/grpc/grpc-web/issues/1285 -- where we're no longer exposing its API due to internal code size issue (PR here).

But we could explore our options there too..

In the meanwhile, maybe you could rebase your code onto 1.4.0 and see if it breaks you?

Thanks!

pro-wh commented 1 year ago

interesting, thanks for the heads up. let me experiment with the changes in 1.4.0. if it's only from the typescript headers, we should be fine

pro-wh commented 1 year ago

update: yes, it does break our .getName callsite. we will be switching back to .name when we upgrade to 1.4.0

sampajano commented 1 year ago

update: yes, it does break our .getName callsite. we will be switching back to .name when we upgrade to 1.4.0

aha interesting thanks for confirming! So i guess that would work but is somewhat a "hack".. :)

pro-wh commented 1 year ago

I'm getting this when I try to run the tests

[16:40:38] I/update - chromedriver: file exists /home/wh0/work/ekiden/grpc-web/packages/grpc-web/node_modules/webdriver-manager/selenium/chromedriver_93.0.4577.63.zip [16:40:38] I/update - chromedriver: unzipping chromedriver_93.0.4577.63.zip [16:40:38] I/update - chromedriver: setting permissions to 0755 for /home/wh0/work/ekiden/grpc-web/packages/grpc-web/node_modules/webdriver-manager/selenium/chromedriver_93.0.4577.63 [16:40:38] I/update - chromedriver: chromedriver_93.0.4577.63 up to date [16:40:38] I/update - selenium standalone: file exists /home/wh0/work/ekiden/grpc-web/packages/grpc-web/node_modules/webdriver-manager/selenium/selenium-server-standalone-3.141.59.jar [16:40:38] I/update - selenium standalone: selenium-server-standalone-3.141.59.jar up to date [16:40:54] I/launcher - Running 1 instances of WebDriver [16:40:54] I/direct - Using ChromeDriver directly... [16:40:54] E/launcher - session not created: This version of ChromeDriver only supports Chrome version 93 Current browser version is 106.0.5249.119 with binary path /usr/bin/google-chrome

is it erroneously using my systemwide chrome rather than its own version for testing?

sampajano commented 1 year ago

I'm getting this when I try to run the tests

[16:40:38] I/update - chromedriver: file exists /home/wh0/work/ekiden/grpc-web/packages/grpc-web/node_modules/webdriver-manager/selenium/chromedriver_93.0.4577.63.zip [16:40:38] I/update - chromedriver: unzipping chromedriver_93.0.4577.63.zip [16:40:38] I/update - chromedriver: setting permissions to 0755 for /home/wh0/work/ekiden/grpc-web/packages/grpc-web/node_modules/webdriver-manager/selenium/chromedriver_93.0.4577.63 [16:40:38] I/update - chromedriver: chromedriver_93.0.4577.63 up to date [16:40:38] I/update - selenium standalone: file exists /home/wh0/work/ekiden/grpc-web/packages/grpc-web/node_modules/webdriver-manager/selenium/selenium-server-standalone-3.141.59.jar [16:40:38] I/update - selenium standalone: selenium-server-standalone-3.141.59.jar up to date [16:40:54] I/launcher - Running 1 instances of WebDriver [16:40:54] I/direct - Using ChromeDriver directly... [16:40:54] E/launcher - session not created: This version of ChromeDriver only supports Chrome version 93 Current browser version is 106.0.5249.119 with binary path /usr/bin/google-chrome

is it erroneously using my systemwide chrome rather than its own version for testing?

Aha not sure.. i forgot how i setup my chrome driver.. maybe 93 is a bit too old i can update it soon.. (although i'm using 106.0.5249.119 too and it seems to work..)

Although, if you run the test using ./scripts/run_basic_tests.sh then it'll use docker and will probably work.

Could you try that and see if it works? Thanks :)

sampajano commented 1 year ago

I will cut a new release soon (prob. later this week) with this change. Thanks again! 😃

sampajano commented 1 year ago

FYI a new release with this change is cut here :)

https://github.com/grpc/grpc-web/releases/tag/1.4.2