Add a flag for proto-loader-gen-types to only output the restrictive type and remove the "__Output" from the name
jdforsythe opened this issue · 5 comments
Is your feature request related to a problem? Please describe.
We have several HTTP + gRPC services where we have the same shape objects coming in on requests. We abstract the request/response handling with koa controllers / unary grpc functions, and share the logic after that.
The interfaces we make for use with the koa controller and service functions overlap exactly with the proto messages we write coming from grpc. However the types generated (the "permissive" type) has all properties optional.
There is a "restrictive" type, but it's always the message name with "__Output" on the end, which makes it very long and clunky to use.
We never want to use the permissive type. If we wanted optional properties, we would mark them as optional in the proto.
Describe the solution you'd like
A flag to suppress the output of the "permissive" type and to make the "restrictive" type use the name the permissive type would have.
Describe alternatives you've considered
Telling developers to always import the __Output
restrictive type is what we've been doing, but this has to be enforced in code review. If the permissive type wasn't generated at all, then it removes the need to look out for this.
Additional context
TypeScript doesn't think the types overlap
Proto file:
message UpdateLocationRequest {
int32 locationId = 1;
string name = 2;
string description = 3;
string locationNumber = 4;
string line1 = 5;
string line2 = 6;
string city = 7;
string state = 8;
string postal = 9;
int32 latitude = 10;
int32 longitude = 11;
string timezone = 12;
}
Generated types (from proto-loader-gen-types):
export interface UpdateLocationRequest {
'locationId'?: (number);
'name'?: (string);
'description'?: (string);
'locationNumber'?: (string);
'line1'?: (string);
'line2'?: (string);
'city'?: (string);
'state'?: (string);
'postal'?: (string);
'latitude'?: (number);
'longitude'?: (number);
'timezone'?: (string);
}
export interface UpdateLocationRequest__Output {
'locationId': (number);
'name': (string);
'description': (string);
'locationNumber': (string);
'line1': (string);
'line2': (string);
'city': (string);
'state': (string);
'postal': (string);
'latitude': (number);
'longitude': (number);
'timezone': (string);
}
Our Typescript interface (for koa controllers/service):
export interface LocationUpdate {
name: string;
description: string;
locationNumber: string;
line1: string;
line2?: string;
city: string;
state: string;
postal: string;
latitude: number;
longitude: number;
timezone: string;
}
Usage:
// service function
const updateLocation = async (locationId: number, location: LocationUpdate): Promise<void> => {
// ...
};
// koa controller
const updateLocationController = async (ctx: Context<LocationUpdate, void>) => {
const { locationId } = ctx.params;
await updateLocation(locationId, ctx.request.body);
};
// grpc unary handler
const updateLocationGrpcHandler = async (req: UpdateLocationRequest): Promise<void> => {
const { locationId, ...location } = req;
// type error here
await updateLocation(locationId, location);
};
Type error:
Argument of type '{ name?: string; description?: string; locationNumber?: string; line1?: string; line2?: string; city?: string; state?: string; postal?: string; latitude?: number; longitude?: number; timezone?: string; }' is not assignable to parameter of type 'LocationUpdate'.
Property 'name' is optional in type '{ name?: string; description?: string; locationNumber?: string; line1?: string; line2?: string; city?: string; state?: string; postal?: string; latitude?: number; longitude?: number; timezone?: string; }' but required in type 'LocationUpdate'.ts(2345)
Using restrictive type eliminates the error since the properties are not optional:
const updateLocationGrpcHandler = async (req: UpdateLocationRequest__Output): Promise<void> => {
const { locationId, ...location } = req;
// no more error
await updateLocation(locationId, location);
};
It seems like the main concern here is that the permissive type has the more obvious and simple name for the type, and so it is likely to be used by accident when developers should be using the restrictive type instead. It seems to me that this is a problem that can be solved using the --inputTemplate
and --outputTemplate
generator options: you could make the restrictive type use the exact message type name, as the permissive type currently does, and make the permissive type have a more awkward name to discourage its use. It could have a suffix, as the restrictive type does by default, or it could have a prefix, to make it not show up in autocomplete, or both.
The permissive type allows properties to be optional that aren't marked as optional. That defeats the purpose of strong typing. I don't want it to be difficult to include the permissive types, I want it to be impossible.
The purpose of these types is to describe the behavior of the gRPC APIs. And the fact is that the objects that the gRPC APIs accept as inputs allow any fields to be omitted. This reflects the protobuf message encoding, in which any fields can be omitted, and they are interpreted as having their default values. So it is a maximally strong type for where it is used.
I understand that the protocol allows omitted fields, but in many instances that's undesirable behavior.
For instance, say we have two tables, described by these types:
interface Organization {
id: number;
name: string;
}
interface Location {
id: number;
organizationId: number;
name: string;
}
and say we have a gRPC call to create a location at an organization, omitting the organizationId
makes no sense, whether the protocol allows it or not. No default value makes sense. This is guaranteed to create a failure at runtime, as the database will reject the record. The permissive type will not catch this issue at compile time, leading to a bug waiting to happen at some point in the future.
We can use your suggestion to name the permissive type something that makes it difficult to import, but it won't be impossible to import, meaning we have to check during code review every time the types are imported in any repository, for any gRPC client and server, to make sure they're the correct types and not the incorrect types being aliased.
If we can suppress the permissive types completely, it will be impossible to import them, eliminating the need to remember to check every import during code review for our hundreds of repositories, and eliminating the possibility of runtime bugs completely for missing properties.
As a counterpoint, the HTTP protocol allows me to send any body I want to a POST endpoint, but that doesn't mean we should use type any
for all HTTP POST calls to our APIs and allow it to fail at runtime. We should, and do, use specific restrictive types to ensure that the data we're sending is correct at compile time.
OK, I'm convinced that it is reasonable to add this option. I'll continue the conversation on the PR.