grpc / grpc-dart

The Dart language implementation of gRPC.
https://pub.dev/packages/grpc
Apache License 2.0
861 stars 271 forks source link

Support using grpc & grpc-web in same project, using conditional imports #457

Closed rajveermalviya closed 3 years ago

rajveermalviya commented 3 years ago
Currently build for native platform fails if grpc-web is imported in the project. Dart supports conditional imports, grpc-dart can use them to support using web & native platforms in same project. `dart:http` package uses them and [here's a guide](https://stackoverflow.com/a/58713064/9263423) 2.9.0 ## Repro steps 1. Add support for grpc-web in an already existing native grpc project 1. Build fails Expected result: It should work seemlessly Actual result: Build fails with dart:html not found
mraleph commented 3 years ago

gRPC-Web and gRPC are fundamentally two different protocols so we can't really create a single library which exports one or another implementation conditionally, if one server speaks gRPC-Web it might not be able to speak gRPC and vice versa.

It's on the user to use conditional imports in the right way in their code to handle the difference between web and native builds.

Another solution is to have grpc-web implementation that works both in the browser and when running natively (see #216).

lukepighetti commented 3 years ago

Is there no technically feasible way to have a single gRPC library (I don't care if it's gRPC or gRPC-web) that works on all dart platforms? I think that's the core issue here.

I was going to go with gRPC for some Flutter tooling I'm building but instead of dealing with a fragmented package I'm probably going with http + web_socket_channel because the cross platform support is well established.

EDIT: here's some reading for those of you who (like me) are learning about this stuff https://grpc.io/blog/state-of-grpc-web/

mraleph commented 3 years ago

@lukepighetti it is possible to provide a gRPC-Web implementation that works on native Dart (if you are fine with its limitations), that's what #216 is about.

You can also conditionally import gRPC-Web or gRPC depending on the platform and create appropriate client channels (potentially talking to different servers). I just think that providing this out of the box in grpc package is not a good idea because it creates confusion (the same library starts talking two different protocols). Though maybe this can be made obvious on the API level.

rajveermalviya commented 3 years ago

@mraleph Is it possible to create a flutter example that demonstrates importing grpc & grpc-web using conditional imports, in examples/ directory?

mraleph commented 3 years ago

@mraleph Is it possible to create a flutter example that demonstrates importing grpc & grpc-web using conditional imports, in examples/ directory?

I can try to create one, but I can't promise I will have time.

I would be happy to merge a PR with such an example though.

Unfortunately managing grpc-dart repo is just a side thing for me.

lukepighetti commented 3 years ago

Moving some conversation to here: https://github.com/grpc/grpc-dart/issues/216#issuecomment-756591725

morgwai commented 3 years ago

@mraleph How about something like this: https://github.com/morgwai/grpc-dart/commit/3606b9f099011026ea4c7292106242ed25c569f8 GrpcOrWebClientChannel uses HTTP/2 gRPC protocol wherever possible (everywhere except web) and gRPC-web on web. The name of the class makes it clear that either of protocols may be used to avoid confusion. If I get a positive feedback, I will create a pull request.

Cheers!

morgwai commented 3 years ago

to be even more explicit and avoid confusion, constructor requires both ports (grpc and grpc-web) to be supplied and checks that they are different: https://github.com/morgwai/grpc-dart/commit/f21748d50f6e31ebb5c5f4fe026d1686d989ade3

mraleph commented 3 years ago

@morgwai I think that looks reasonable. Please do send a PR, just make sure to include the tests that cover both VM and browser platform. Thanks! Maybe NativeOrWeb is a better name, but I don't have strong opinion about it.

[I am away most of this week - so I can't promise a timely review]

morgwai commented 3 years ago

Thanks :) I'll work on tests and examples. Don't worry about slow reviews: I will also need some time to finish it ;-)

Regarding the name: it's also bothering me, but I really think that Grpc should be somewhere in there (to be honest, I hate that standard gRPC channel is called just ClientChannel instead of GrpcClientChannel ;-] For ppl who are unfamiliar with dart gRPC API and don't know the context of a given code they look at, it makes it much harder to understand what that thing is). I think the most descriptive is GrpcOrGrpcWebClientChannel : initially I was concerned that it's too long, so I've shortened it to the current GrpcOrWebClientChannel, but today I've just started to work on a mechanism that uses either gRPC or websocket (as a separate library, to support client streaming, similarly to red-hat's gRPC anywhere) and I will have probably a class named GrpcOrWebsocketClientChannel, which is dangerously similar ;-] Therefore, I would opt for the full GrpcOrGrpcWebClientChannel if you don't mind ;-)

Cheers!

morgwai commented 3 years ago

@mraleph https://github.com/grpc/grpc-dart/pull/479

morgwai commented 3 years ago

Merged, huh huh :) Many thanks to @mraleph for the review!

loeffel-io commented 1 year ago

import 'package:grpc/grpc_or_grpcweb.dart' is amazing! @morgwai but things get tricky again when i need to use the WebCallOptions