Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a flag for proto-loader-gen-types to only output the restrictive type and remove the "__Output" from the name #2839

Open
jdforsythe opened this issue Oct 10, 2024 · 5 comments · May be fixed by #2840

Comments

@jdforsythe
Copy link
Contributor

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);
};
jdforsythe added a commit to jdforsythe/grpc-node that referenced this issue Oct 10, 2024
@jdforsythe jdforsythe linked a pull request Oct 10, 2024 that will close this issue
@murgatroid99
Copy link
Member

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.

@jdforsythe
Copy link
Contributor Author

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.

@murgatroid99
Copy link
Member

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.

@jdforsythe
Copy link
Contributor Author

jdforsythe commented Oct 16, 2024

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.

@murgatroid99
Copy link
Member

OK, I'm convinced that it is reasonable to add this option. I'll continue the conversation on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants