bufbuild / protobuf-es

Protocol Buffers for ECMAScript. The only JavaScript Protobuf library that is fully-compliant with Protobuf conformance tests.
Apache License 2.0
1.14k stars 68 forks source link

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

Open lourd opened 1 week ago

lourd commented 1 week ago

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 commented 1 week ago

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 commented 1 day ago

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