grpc_or_grpcweb.dart: should this be exporting shared types?

[gawmanarnar]

If I want to use ResponseStream or GrpcError in my application I need to include the correct header so I end up with imports like this:

import 'package:grpc/grpc_or_grpcweb.dart';
import 'package:grpc/grpc.dart'
    if (dart.library.html) 'package:grpc/grpc_web.dart';

which seems a bit silly, could/should grpc_or_grpcweb.dart export these shared types?

[gustav3d]

Exactly. The current design makes no sense at all. Also the new design, with its 2 constructors , instead of one doing it all (just document or name default parameters so its obvious some only work in non web etc...), is done a by somebody confused.

[mraleph]

@gustav3d regarding multiple constructors - my reasoning here goes like this:

Originally contributed code asked to specify both endpoints even if they are the same, which seemed inconvenient for users like you who have a server capable of talking both protocols.

The design with multiple constructors makes it clear what each of those constructors does and reduces possibility of error.

With a single constructor it is a bit hard to make a sensible default without resorting to marker values for optional parameters, e.g. I considered suggesting this:

/// [grpcWebHost] defaults to [grpcHost] if empty. 
/// [grpcWebPort] defaults to [grpcPort] if `0`.
GrpcOrGrpcWebClientChannel({
    required String grpcHost,
    required int grpcPort,
    String grpcWebHost = '',
    int grpcWebPort = 0,
})

But this looks pretty strange and would lead to incorrect behaviour if user accidentally passes '' or 0, due to a bug in their code. Another option is to do:

/// [grpcWebHost] defaults to [grpcHost] if `null`. 
/// [grpcWebPort] defaults to [grpcPort] if `null`.
GrpcOrGrpcWebClientChannel({
    required String grpcHost,
    required int grpcPort,
    String? grpcWebHost,
    int? grpcWebPort,
})

But it has the same problem: user can accidentally pass null, due to a bug, and behaviour would be wrong.

Hence I opted for suggesting to use multiple constructors which make user intent explicit and reduce possibility of incorrect configuration.

[gustav3d]

I believe its conext dependent if null parameters are a real problem, danger or not. In this case i argue it is not, 99% of cases needing different web parameters should be handling those as non null in their code anyohw?. And if only one of the web parameters is null, we can throw. If both are null by mistake, the user should perhaps change them to be non null.

[gawmanarnar]

487