Open peteringram0 opened 4 years ago
same problem here
We have a patch in google-closure to support service workers (via fetch) and will try to get the change to github soon.
@wenbozhu has this been resolved yet?
@travikk if it's any use to hear, we've been using grpc-web in web workers without a problem.
@travikk if it's any use to hear, we've been using grpc-web in web workers without a problem.
How so? The generated code references window object which is not available in web worker. @markns
Same problem, I moved forward a bit with using
transport: grpc.FetchReadableStreamTransport({}),
since workers support fetch,
but I'm getting:
Fetch.catch Fetch API cannot load: grpc:
and then Response closed without headers
ugh an no way of knowing how else to proceed
@altryne FYI there's a recent discussion going on in https://github.com/grpc/grpc-web/issues/1277 that will probably bring a solution here. You could consider follow it :)
It looks like the gRPC client accepts an instance of XhrIo
:
Which accepts an XmlHttpFactory
, and there is an implementation of that using the fetch API.
It seems to me that if one can get an instance of XhrIo
initialized with FetchXmlHttpFactory
, it will work. Unless there's some big part of the implementation that is inconsistent with XHRs. I can see that at one point this was planned for development, but never finished:
After finishing the implementation so that it reads workerScope
from the options and instantiates XhrIo
with FetchXmlHttpFactory
using that workerScope
, it appears to work just fine. A simple unary RPC call is finished with no problem inside of a service worker:
However, I haven't tested server-side streaming using this mode.
If I open a PR to upstream this change, would that be helpful to push this along? Even if there are some features not supported using fetch, those could be noted as a caveat and with helpful error messages explaining why.
@tjhorner Hi! Thanks for your interest and digging here! Much appreciated!
Actually, i've realized that this feature was implemented on the internal fork but not open sourced..
It should actually be quite an easy change!
However, I haven't tested server-side streaming using this mode.
There's another option called useFetchDownloadStreams
as below:
Our internal implementation is quite simple too. Basically replacing the following line:
with something like this:
newXhr(isUnary) {
const useBinaryChunks = this.chunkedServerStreaming_ && !isUnary;
if (this.workerScope_ || useBinaryChunks) {
const xmlHttpFactory = new FetchXmlHttpFactory({
worker: this.workerScope_,
streamBinaryChunks: useBinaryChunks,
});
return new XhrIo(xmlHttpFactory);
}
return new XhrIo();
}
If I open a PR to upstream this change, would that be helpful to push this along? Even if there are some features not supported using fetch, those could be noted as a caveat and with helpful error messages explaining why.
Thank you for the offering here! Appreciate it! I think it can certainly help move things along here!! 😃
The only thing is, we're currently in the process of doing a Typescript migration, so it might not be a great timing to take PRs if this takes an extended amount of time. In that case it would be better to wait until the migration is over before we do this. But if it's a really simple change, i don't mind taking it up sooner too! 😃
@sampajano Thank you for the very quick response!
My solution was remarkably similar, minus the streamBinaryChunks
option:
I can definitely submit a PR if you think it's a small enough change. But if I do, something I would like more info on is testing strategy. Not sure how thorough you'd want the tests for this (e.g., should fetch
also be mocked similar to the existing tests with XHR, etc.)
There's no rush on my end, however. For the project we're using this for at work, we are vendoring the library with these changes, so I'm ok with waiting for a more official release in the meantime.
Let me know what you think!
My solution was remarkably similar, minus the streamBinaryChunks option:
Oh great! Glad to note! :)
Could you try passing the useFetchDownloadStreams
option as well (which passes streamBinaryChunks
to the underlying factory)? I think it's more efficient. If it works i don't mind just leave it on by default :)
I can definitely submit a PR if you think it's a small enough change. But if I do, something I would like more info on is testing strategy. Not sure how thorough you'd want the tests for this ...
Really appreciate the considerations! Yes def agreed that we should ensure it's tested 😃
Luckily we have an interop test already i think you can probably easily reuse! (no need for mocking! :)
I think you can just:
Copy the following block and add the --use_fetch
option to npm test
And add a new option in the test (around here) that looks like this:
if (argv.use_fetch) { ... }
Where you'd set a global constant or something to pass the new fetch option to all new TestServiceClient(...)
calls.
If you don't mind, could you try the above and see how it works? Lemme know if you run into any issues.
Thanks! 😃
Im trying to run GRPC from inside a service worker. It works fine from inside the main application thread and only throws an error from inside a service worker.
Error:
Im assuming this is due to how GRPC communicates to the backend services and assuming this is not possible to do at the moment?
Thanks