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

Feedback/feature request on v2: make inclusion of $typeName optional #994

Open
lourd opened this issue Oct 22, 2024 · 5 comments
Open

Feedback/feature request on v2: make inclusion of $typeName optional #994

lourd opened this issue Oct 22, 2024 · 5 comments

Comments

@lourd
Copy link

lourd commented Oct 22, 2024

Hey y'all, wanted to share some feedback after trying out the connect-es v2 RC and protobuf-es v2, attempting to update my projects.

This statement from the manual is proving to be very untrue for me in practice:

You can replace PlainMessage<...> type with User

This is due to the inclusion of the new $typeName property. This breaks my types in many places. Everywhere I was using implicit structural type matches is now broken, in addition to every place where I was using PlainMessage. There's not a single use of PlainMessage<Msg> I could simply replace with the Msg type itself. Resolving these cases unfortunately is not as simple as just using Omit it due to nested messages.

In my experience, having worked at Google and used the internal protobuf toolchain for TS extensively, the greatest strength of Connect has been the interface flexibility for Messages. You didn't have to use the classes, but you could; PlainMessage was incredibly useful. Now you basically have to use the create function, or otherwise manually include a $typeName property when making objects. Now I'm having to include the schema in places I wasn't before, increasing my code size, or I'm adding a property in many places that I'll have to go and manually update whenever I decide to move or rename a proto.

For example, I have a front-end API server that has several RPCs that are effectively proxies to RPCs on a microservice. The front-end server validates the user and then passes the request along to the microservice, and returns the response from the microservice. Now I have type errors at the site of passing the request and on returning the response back from the microservice to the client.

I understand that some folks want nominal typing, that the flexibility is a footgun they don't want. I hope there's a compromise that can be made.

@lourd
Copy link
Author

lourd commented Oct 22, 2024

I would argue that this behavior is one of the unique strengths of TypeScript:

class Foo {
    bar: string= ""
}


class Baz {
    bar: string = ""
    quux: string = ""
}

let f: Foo = new Baz()

and breaking with that will do more harm than good. It should be easy to use your protobuf message types as interfaces, as if they were any other interface, and $typeName immediately gets in the way of that.

@wenerme
Copy link

wenerme commented Oct 31, 2024

atleast provide a way to get the typeName, like

export const WriteResponseSchema: GenMessage<WriteResponse> & {
 typeName: "wener.wode.fs.v1.WriteResponse"
} = /*@__PURE__*/
  messageDesc(file_wener_wode_fs_v1_file_system_service, 12)

so I can use WriteResponseSchema.typeName to resolve the type error

@timostamm
Copy link
Member

#1016 asks to remove $typeName and $unknown from the types.

@timostamm
Copy link
Member

Requiring this property isn't ideal for many reasons, but nominal typing isn't the only reason for the $typeName property. It's also used to distinguish between a complete message and a partial init object (avoiding a big perf hit), it prevents passing a wrong message in the reflection API, and it's necessary for isMessage(), which we need to ease migration.

atleast provide a way to get the typeName
[...]
WriteResponseSchema.typeName

I'm bummed we missed that. I think it might be possible to type the typeName without breaking users.

@coobeeyon
Copy link

Is there a less clunky way to get the typeName at present than this?
const typeName = create(schema, {}).$typeName;

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

4 participants