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

V2: export type MessageInit #941

Closed
ngbrown opened this issue Jul 23, 2024 · 5 comments
Closed

V2: export type MessageInit #941

ngbrown opened this issue Jul 23, 2024 · 5 comments

Comments

@ngbrown
Copy link

ngbrown commented Jul 23, 2024

In V2, there is a type, MessageInit that excludes $typeName (and $unknown) and when passing objects to functions that will call create() it is useful to use a derived type like that.

Currently in V2, I have to do something like this: MessageInitShape<typeof MyTypeSchema>, but it would be shorter to do this: MessageInit<MyType>.

So this request is to export the type MessageInit so it can be imported.

Also, having simpler types available might speed up the TypeScript language service, since it seems to have gotten fairly slow while I was trying out the V2 beta.

@timostamm
Copy link
Member

Hey Nathan,

we don't export MessageInit for forward compatibility. Imagine that we want to accept field representations in create() that cannot be inferred from the generated type. MessageInitShape allows us to attach a separate type for init to the descriptor, and use it for create().

We expect that you would typically want to write a generic function that works with arbitrary messages. We have an example in the v2 manual:

import { getOption, type DescMessage, type MessageShape } from "@bufbuild/protobuf";
import { reflect } from "@bufbuild/protobuf/reflect";
import { sensitive } from "./gen/example-option_pb";

export function redact<Desc extends DescMessage>(
  schema: Desc,
  message: MessageShape<Desc>,
) {
  const r = reflect(schema, message);
  for (const field of r.fields) {
    if (getOption(field, sensitive)) {
      // This field has the option (example.options.sensitive) = true
      r.clear(field);
    }
  }
}

Tip

  • EnumShape extracts the enum type from an enum descriptor.
  • MessageShape extracts the type from a message descriptor.
  • MessageInitShape extracts the init type from a message descriptor - the initializer object for create().

The function could also take an init object with MessageInitShape<Desc>. For this pattern, the types are pretty concise - MessageInit<MessageShape<Desc>> would actually be more verbose.

But your function is a different case, it's specific to one message. I'd love to better understand the use case. What kind of function are you using it for?

@ngbrown
Copy link
Author

ngbrown commented Jul 24, 2024

Hi Timo,

Thanks for explanation. I'm not sure I totally understand your reasoning though. The message descriptor is really doing a lot because it also has the JSON type reference (when the json_types=true option is turned on). I also get errors because the MessageInitShape is doing too much. See my examples below.

Since filing this I turned on the JSON generated types because in my application, the Node.js backend-for-frontend has to communicate with the front-end by serializing via JSON. I had to turn it on because of bigint, but I see that enums also change. I'm using Remix, so while the current version still needs this, the future version will communicate with turbo-stream, so I can use bare JS types again, so I might drop the json conversion at that point.

That is some background on the application, but for the specific use case where I would use MessageInit, there's actually two situations.

First scenario is where a UI component would normally be initialized with a message, but if that isn't available, or I'm manipulating the values for display (and pending UI), I want to just manipulate the relevant values without worrying about the $typeName value:

export type IProtoGearRatioLimit = MessageInit<ProtoGearRatioLimit>;
export type RequiredNonNullable<T> = { [P in keyof T]-?: NonNullable<T[P]> };
export type GearRatioLimit = RequiredNonNullable<IProtoGearRatioLimit>;

const [gearLimits, setGearLimits] = useState<GearRatioLimit[]>([
  { gear: 1, highLimit: 207.1, lowLimit: 121.8 },
]);

When I use export type IProtoGearRatioLimit = MessageInitShape<ProtoGearRatioLimitSchema>;, then I get errors like this:

error TS2339: Property 'lowLimit' does not exist on type 'GearRatioLimit'.
Property 'lowLimit' does not exist on type 'RequiredNonNullable'.

and

error TS2339: Property 'lowLimit' does not exist on type 'GearRatioLimit'.
Property 'lowLimit' does not exist on type 'RequiredNonNullable'.

Second scenario is the return type of a bunch of my functions because I want to do a type union on whether a specific field is filled out or not. I have something like the following:

export type IProtoRpcRequest = MessageInit<ProtoRpcRequest>;

interface RequiredRpcRequestRouting {
  standId: number;
  type: ProtoRpcRequestType;
}

export type RpcRequestWithRouting = IProtoRpcRequest &
  RequiredRpcRequestRouting;

Then functions that create the basics of a message:

export function createDataCollectionStartManualRecordCommand(
  standId: number
): RpcRequestWithRouting {
  return {
    standId,
    timestamp: getTimestamp(),
    type: ProtoRpcRequestType.DATA_COLLECTION_START_MANUAL_RECORD_COMMAND,
    dataCollectionStartManualRecordCommand: {},
  };
}

Then finally, a function that needs a specific option if the specific fields to derive that information information are not filled out (or not available on that type):

export interface RpcClientFunc {
  (
    request: RpcRequestWithRouting,
    options?: RpcClientFuncOptions
  ): Promise<ProtoRpcResponse>;
  (
    request: IProtoRpcRequest,
    options?: RpcClientFuncOptions & { rpcTopic: string }
  ): Promise<ProtoRpcResponse>;
}

If I use export type IProtoRpcRequest = MessageInitShape<ProtoRpcRequestSchema>; in this last scenario, I get errors like this:

Property 'standId' does not exist on type 'Record<string, unknown> | Message | MessageInit | RpcRequestWithRouting'.
Property 'standId' does not exist on type 'Message'.

and

error TS2345: Argument of type 'Record<string, unknown> | Message | MessageInit' is not assignable to parameter of type 'ProtoRpcRequest | MessageInit | undefined'.
Type 'Message' is not assignable to type 'ProtoRpcRequest | MessageInit | undefined'.
Type 'Message' is not assignable to type 'ProtoRpcRequest'.
Type 'Message' is not assignable to type 'Message<"hdi.vts.v1.ProtoRpcRequest">'.
Type 'string' is not assignable to type '"hdi.vts.v1.ProtoRpcRequest"'.

Note: I used patch-package to force the MessageInit to be exported. Maybe that's why I got myself into this situation, but it also was the least change from the protobuf.js and the protobuf-es V1 implementation (with PlainMessage<Proto...>).

@timostamm
Copy link
Member

I'm not sure I totally understand your reasoning though.

MessageInit makes all properties optional, so that you can pass only relevant data to create(). To do so, we use a mapped type.

It's awesome that TypeScript's type system is powerful enough to do things like that, but there are limits to the approach. Imagine that we need to change the behavior for specific fields in create(). For example, to make proto2 required fields required. This isn't possible with a mapped type because the property for a proto2 required field has the same type as the property for other fields.

The solution is to generate an init type for such a message, and to attach it to the descriptor, so that it can be used in create(). If we export MessageInit, this solution no longer works, because MessageInit does not know about the new generated type. We don't have concrete plans to use it at this point, but I hope you understand we'd like to keep this door open if possible.

I also get errors because the MessageInitShape is doing too much.

MessageInitShape does very little. MessageInit is the complex mapped type (it was called PartialMessage in v1).

I've pushed up your example in 4f2df57, but I can't reproduce the error. I wonder if your first scenario could be simplified? Messages are no longer classes with v2, and the goal is to make the generated type more versatile:

import type { ProtoGearRatioLimit } from "./gen/ts/extra/issue941_pb.js";

const initialState: Omit<ProtoGearRatioLimit, "$typeName">[] = [
  {
    gear: 1,
    highLimit: 2,
    lowLimit: 3,
  },
];

This doesn't require any mapped types except for the built-in Omit. The $typeName property has upsides and downsides, but it is necessary to solve the problem described in #551 and connectrpc/connect-es#694.

I'm not sure I fully understand your second and third scenario, but it looks like you could straight-up use the generated type here as well? See dae4d47. Of course you'll need a mechanism to serialize the message for the BFF, but this approach should work well with proto3 messages (I understand that $typeName can still be a bit awkward).

Do you aim to only use types in the frontend for small bundle sizes?

@ngbrown
Copy link
Author

ngbrown commented Jul 25, 2024

Thanks for trying to replicate the issue in the tests.

The weird thing is, that in my examples above, I had:

export type IProtoRpcRequest = MessageInitShape<ProtoRpcRequestSchema>;

I copied that right from my file and there was no TypeScript error on that line. I noticed your test had something different so I realized that I should have included the typeof: MessageInitShape<typeof ProtoRpcRequestSchema>.

I would have expected TypeScript to complain about using a constant where it is expecting a type. MessageInitShape without a typeof generates TypeScript errors in your test project, but doesn't in my project. I don't see anything that is significantly different in that regard. We have the same version of "typescript", 5.5.3.

When I was responding to you I quickly switched back to MessageInitShape from MessageInit to see if it still worked and I didn't re-add the typeof. I knew that at one point, because I mentioned it in the original issue.

When adding typeof I've resolved all the errors I listed above.

For the second scenario, I initially did call create in each message creation function and return the fully formed ProtoRpcRequest, but I realized that then no type information on whether the fields were fully filled out or not would pass through.

The front-end is also directly serializing and de-serializing protobuf messages. I'm trying to add a gRPC service client to the back-end, so then there's the need to transport the resolved messages, so that's how the JSON serialization became a concern.

Anyways, sorry about this chasing of errors. If the exporting of MessageInit is a no-go, then you can close the issue.

@timostamm
Copy link
Member

I'm stumped why TypeScript didn't provide a helpful error message 🤷
The generated ProtoRpcRequestSchema is a value, so typeof is necessary to use its type as a type parameter.

For the second scenario, I initially did call create in each message creation function and return the fully formed ProtoRpcRequest, but I realized that then no type information on whether the fields were fully filled out or not would pass through.

Yeah, in Protobuf, fields are basically always optional. It's an idiom that doesn't always match well with type systems with nullable types. We plan to provide some more options for use cases like this with protovalidate.

If the exporting of MessageInit is a no-go, then you can close the issue.

We'll better hold off in it for now 👍

Thanks for opening the issue!

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

No branches or pull requests

2 participants