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

Create raw data types as well #1058

Open
abalmos opened this issue Jan 22, 2025 · 7 comments · May be fixed by #1060
Open

Create raw data types as well #1058

abalmos opened this issue Jan 22, 2025 · 7 comments · May be fixed by #1060

Comments

@abalmos
Copy link

abalmos commented Jan 22, 2025

syntax = "proto3";
package example;
message User {
  string id = 1;
}

will produce something like

export type User = Message<"example.User"> & {
  /**
   * @generated from field: string id = 1;
   */
  id: string;
}

However, the User type is hard to use when working with the message body as data throughout the code based because of the $typeName and $unknown fileds that Message<"example.User"> adds. What if instead it generated something like:

export type UserRaw = {
  /**
   * @generated from field: string id = 1;
   */
  id: string;
};

export type User = Message<"example.User"> & UserRaw

So that UserRaw can be used to hold data in the program logic, but one can also easily do:

const user: UserRaw = { id: "123" };
create(UserSchema, user)

This would remove the need for using a helper like OmitTypeName all over the place.

@timostamm
Copy link
Member

We didn't anticipate the dislike for $typeName. We hear you, but it isn't going to be easy to fully support the removal, and there are downsides too. Because of this, we won't be generating a "raw" type in the near future.

There is an alternative, though:

The source code of protoc-gen-es is pretty compact, considering that it generates enums, types, descriptors, and JSON types. If you pick just the code to generate message types, it should be less than 500 lines of code. So my suggestion would be to create such a plugin, and give it a try. If it works out well, you could publish it for others to use.

@abalmos
Copy link
Author

abalmos commented Jan 25, 2025

@timostamm It is unfortunate that some folks dislike $typeName. Personally, I’ve found it quite useful for making our APIs both generic and strongly typed. However, I’m not sure how that relates to the types exported by protoc-gen-es.

Perhaps I wasn’t clear initially. I have no interest in removing $typeName. However, as it stands, protoc-gen-es does not produce types that truly map to the protobuf definitions themselves. In other words, on the application side, we lack types to represent the data defined by protobuf messages. Instead, the only exported types reveal internal details ($typeName and $unknown) of the protobuf-es implementation.

The type I’m looking for is already generated by protoc-gen-es, but it’s only included as part of a union. To use it, we have to wrap the exported type with another type that strips out the other union members. This is not ergonomic, and seemingly the only part of the design that isn't.

The change is straightforward -- I’ve submitted #1060 showcasing one way of doing it. I don’t see how this could be a breaking change, and as far as I can tell, all project tests pass with the modification.

Side note: I do think it would be more correct if User (using the example from this issue) referred to the plain object itself (as it actually represents the User data structure), while a new type, maybe, UserMessage could represent the union of that plain/raw type with the Message<> internal details of protobuf-es. However, at this point, that would be a breaking change for existing users.

@Yovach
Copy link

Yovach commented Jan 29, 2025

In my case, I'd like to create a function like :

// user.proto
syntax = "proto3";
package example;
message User {
  string id = 1;
}
// index.ts
function createMyThingByUser(user: User) {
// ...
}

But the $typeName makes this thing hard because it implies the use of create or adding $typeName to the object used as parameter which is not ideal.

const userId = 1;
createMyThingsByUser(create(UserSchema, { id: userId }));

I totally agree with what @abalmos said (UserMessage)

@nicksnyder
Copy link
Member

Going to mark this issue as closed since we do not have any plans to introduce a new type like what was requested.

The $typeName field is what signals we have a complete type, rather than a partial type used for initialization. If we introduce a new "raw" type, it will not be easy to differentiate between a complete "raw" type and a partial type.

Anyone who is ok with the tradeoffs, can write a custom code generation plugin to generate exactly what they want, as mentioned in #1058 (comment).

@nicksnyder nicksnyder closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2025
@abalmos
Copy link
Author

abalmos commented Jan 31, 2025

@nicksnyder These types would export as different files, yeah? I couldn't see a way to do it otherwise.

While $typeName means something to this library, it means nothing outside of it and is clearly a leaked abstraction. I don't see how it serves any purpose to exist in the types themselves. We are not asking for a new type, just for this project to export a type it already generates but does not name.

It is rather unfortunate that the project does not seem interested in user feedback.

@nicksnyder
Copy link
Member

I was triaging issues with @timostamm this morning and was trying to be helpful by providing an update that we aren't optimistic about this making sense to do, but in retrospect I clearly made a mistake here in closing this issue before more discussion happened. I am sorry!

I am going to re-open this for now and @timostamm can share more detailed thoughts about the potential consequences of exposing a separate type.

We do very much care about user feedback (and the API) and I am sorry I gave the impression otherwise. There are many tricky issues to navigate when it comes to being true to the semantics of Protobuf while providing the best API/devex possible and being mindful of performance all at the same time.

@nicksnyder nicksnyder reopened this Jan 31, 2025
@anthonysessa
Copy link

Currently a separate type via json_types is being created for the protos, so the discussion about raw types is very much related to that, and the discussion I am proposing to have is to update json_types to respect optional vs non-optional proto definitions.

I think we end up with a solution that does not add any new typings but achieves what is being requested both in this discussion and #1062.

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.

5 participants