[Transport] React Native support - Adds and exposes ReactNativeTransport.

[pbsf]

This revision adds and exposes a ReactNativeTransport, that uses an ArrayBufferXhrTransport. Using ArrayBufferXhrTransport, React Native clients are able to work with gRPC-WEB.

Verification

Manual verification with a React Native toy sample using Expo, which I intend to submit on a separate PR.

[jonny-improbable]

Does this imply that the progress event does not fire? It appears that you are only processing the response data when the XHR has completed (ie: when the loadend event is fired).

[pbsf]

Exactly, this event does not fire. #141 (comment) is a similar solution to what this PR tries to achieve, and this comment also mentions that this event is not being fired.

[jonny-improbable]

Thanks for clarifying :)

[jonny-improbable]

Thanks for raising this @pbsf - if we are going to make this part of the @improbable-eng/grpc-web package then we will need to find a testing strategy to ensure it doesn't randomly break for consumers in future - is this something you can help with?

[pbsf]

@jonny-improbable, thank you for reviewing this PR. I had plans to add an example similar to grpc-web-react-example, which would be named grpc-web-react-native-example. This example would be very similar to the React one, but with a React Native application (similar to https://snack.expo.io/Skvv85Usx, but with gRPC-WEB instead of a Hello World). I'm not sure when I'll have time to do so.

But after your comment I noticed the integration_test folder. How about changing https://github.com/improbable-eng/grpc-web/blob/0c7a81a25d11e97972b5b6e6e61312fd7ad6413a/integration_test/ts/src/testRpcCombinations.ts by adding the introduced ArrayBufferXhrTransport into the function runWithSupportedTransports? That seems much easier to do and validate. Thoughts?

[jonny-improbable]

@pbsf

But after your comment I noticed the integration_test folder

I think a test provides far more value, in terms of long term stability/maintainability than an example, so I would rather we start there. The integration tests were designed to be extended; much in the same way that the NodeHttpTransport is special cased.

With regards to your implementation, one potential issue is that the progress event is still being listened for by the base XHR class which your ArrayBufferXHR extends; whilst this shouldn't cause any issues it's not ideal.

After speaking with @MarcusLongmuir we both feel that this transport may be better implemented as a standalone package within this repo, in the same vein as the grpc-web-node-http-transport package. Whilst this would incur a little duplication of the underlying XHR base class logic, we believe it will be more maintainable in the long term.

Please let me know your thoughts, and I'm happy to help :)

[jonny-improbable]

• extract into a new package
• add integration tests

[jonny-improbable]

Hi @pbsf, please could you provide an update on the status of this PR? Thank you :)

[pbsf]

Hi @pbsf, please could you provide an update on the status of this PR? Thank you :)

I was able to split the ArrayBufferXHR in its own package, but I'm having troubles to run the integration tests on my environment, thus I'm currently unable to add the integration tests needed. I ended up getting the Karma server to work, but after visiting localhost:9876 nothing happens. I installed the certificate on my browser. Do I need to do so on my OS as well?

I created a Docker image to run the integration tests, and with it I can reproduce my issues. Once improved, that image can help others that wish to contribute to this project. I think that image should be on a separate pull request.

[jonny-improbable]

I think it would be a good idea to suggest when/why someone might wish to use this transport. Also, perhaps grpc-web-react-native-transport would be a more descriptive name?

[sebleclerc]

I have cloned the repo and give that XHR transport a try and it's working!

In our react native setup, we had used used this method in the past to fix XHR errors but I wasn't able to make it work anymore.

I installed the package from local file and everything is working good. Thanks in advance to make it available in an official NPM package 😄

[jonny-improbable]

Hey @pbsf, please could you rename the package to grpc-web-react-native-transport and we will be good to merge :)

[pbsf]

@sebleclerc, I’m happy that this helped you!

@jonny-improbable, I’ll do the renaming and resubmit tomorrow. I wasn’t able to run the integration tests as I mentioned on my comment above, though.

[jonny-improbable]

Not a huge fan of the duplication; however we should be able to solve that by extracting detach and the base XHR Transport into some kind of gprc-web-internal-utils package in future.

[jonny-improbable]

This is the only thing I'm not happy with.

AFAIK this is the only change from the XHR class copied from transports/http/xhr.ts which means it could easily be lost if we were ever de-dupliate the implementations.

Could you instead please restore the original onProgressEvent() implementation here, mark the method as protected in this base XHR class, and then override it with an empty implementation in ArrayBufferXHR implementation.

Thanks.

[jonny-improbable]

One last change wrt avoiding potential future problems when refactoring and then we can land it.

[pbsf]

@jonny-improbable, are there any more changes needed before merging this? Thanks.

[jonny-improbable]

Sorry for the delay on this, I had hoped to land #539 first, but my time to contribute toward this package has dwindled over the last few months.

[consultchwong]

It seems grpc-web-react-native-transport is pending for submission to npmjs. Meanwhile, any method to use reactnativetransport. I somehow need it urgently. See if anyone can help. Thanks.

[sebleclerc]

@consultchwong I did checkout the PR branch and point my package.json to the local file. Since this PR is merge, you can probably checkout develop or master

[pbsf]

I agree that a submission to npmjs would be great. I created #617 stating that.