Adding Explicit Type Definitions to Factory Method Objects

[sam-step]

Hi, I recently ran into the "inferred type of this node exceeds the maximum length" error referenced in #449

TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.

It can be easily reproduced by running ts-proto on google/protobuf/descriptor.proto.

I understand turning off exact types works around the problem. However, the exact types feature is pretty nice so I'm looking to see if there are alternative solutions.

Following the compiler's suggestion to add explicit annotations seems to address the problem. E.g.

-export const FileDescriptorProto = {
+export const FileDescriptorProto: {
+    encode(message: FileDescriptorProto, writer?: _m0.Writer): _m0.Writer;
+    decode(input: (_m0.Reader | Uint8Array), length?: number): FileDescriptorProto;
+    fromJSON(object: any): FileDescriptorProto;
+    toJSON(message: FileDescriptorProto): unknown;
+    create<I extends Exact<DeepPartial<FileDescriptorProto>, I>>(base?: I): FileDescriptorProto;
+    fromPartial<I extends Exact<DeepPartial<FileDescriptorProto>, I>>(object: I): FileDescriptorProto;
+} = {
  encode(message: FileDescriptorProto, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {

Is adding support for these explicit annotations something you'd consider (it certainly produces more verbose code...)? If so, happy to take a stab at contributing a PR for it.

[stephenh]

Hi @sam-step ! Apologies for the late reply, but the explicit annotations sounds worth trying!

I was thinking it will blow up the *.ts file output size, but it's just a type, so would get minimized out in terms of actual *.js code.

Do you think doing an interface like const FileDescriptProto: SomeGenericType<FileDescriptorProto> would also work?

Even if SomeGenericType itself was included in the generated output (instead of being imported from an npm library), seems like that would also cut down on the output size quite a bit.

If you're still interested, would be great to have a PR! Thanks!

[sam-step]

Do you think doing an interface like const FileDescriptProto: SomeGenericType would also work?

Yes, I like this idea a lot! Addresses the compilation issue and has the nice side benefit of a much smaller (and more readable) declarations file.

Took a closer look at the code and I think this should be doable. A few questions came up that I want to get feedback on before investing more time here:

Naming
These static object have the same name as their respective messages, so initially Message sounded right. But it feels misleading. It's not really a message, but rather a companion object of static methods, mostly encoding & decoding utilities. StaticMessage? MessageUtilities?

Flag or default?
I'd imaging making this behavior default and avoiding yet another flag (and series of conditionals in the code) would be desired. Assuming it doesn't actually change the semantics of the generated code. But lmk if you think it'd be better to put it behind a flag.

Constant members
With outputTypeAnnotations and outputTypeRegistry flags, the static object has a const $type property. Defining it as a string in the interface would result in reduced type specificity, so I think if either of these flags are set, we'd have to further parameterize the interface, like:

interface StaticMessage<T, N extends string> {
  readonly $type: N
  encode(message: T, writer?: BinaryWriter): BinaryWriter
  decode(input: BinaryReader | Uint8Array, length?: number): T
  // Others...
}

export const FileDescriptorProto: StaticMessage<FileDescriptorProto, 'google.protobuf.FileDescriptorProto'> = {
  $type: "google.protobuf.FileDescriptorProto" as const,
  // ...
}

const type = FileDescriptorProto.$type // Guaranteed to be "google.protobuf.FileDescriptorProto"

Does that make sense? I think this is the only way to retain the same level of specificity here, but I might be missing something.

Conditional methods
If outputExtensions is set, the setExtension and getExtension methods are conditionally added to the static object depending on whether the target message has any extensions defined or not. So a single interface per build/file won't necessarily work.

Assuming these are the only two conditionally added methods, one option could be just create a second interface and union them when needed. E.g.:

interface StaticMessage<T> {
  encode(message: T, writer?: BinaryWriter): BinaryWriter
  decode(input: BinaryReader | Uint8Array, length?: number): T
  // Others...
}

interface Extendable<T> {
  getExtension<V>(message: T, extension: Extension<V>): V | undefined
  setExtension<V>(message: T, extension: Extension<V>, value: V): void
}

const MyMessage: StaticMessage<MyMessage> & Extendable<MyMessage> = {
  // ...
}

const MyOtherMessageWithoutExtensions: StaticMessage<MyOtherMessageWithoutExtensions> = {
  // ...
}

Another route could be to just not conditionally add methods to the object (ie always add get/setExtension methods if outputExtensions is true, regardless of whether the message has any extensions). But it seems wasteful to add methods that'll never get called.

[stephenh]

Naming

Good question, maybe MessageProto or MessageConst or MessageMethods or MessageFns. Maybe MessageFns?

Flag or default

I'm good with default, since it's "just types" and should get stripped out of any bundled code and not affect output size.

constant members

Good catch on the $type and yep makes sense it can be a generic--should work.

conditional methods

Another great catch--the StaticMessage<MyMessage> & Extendable<MyMessage> seems pretty nice to me, if that works out.

Thanks for all of the research into this so far! You've found several things I would not thought of until getting ~1/2th way through the PR. :-)

[stephenh]

🎉 This issue has been resolved in version 2.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release
• v2.2.0

Your semantic-release bot 📦🚀

[novascreen]

@stephenh can we expect this version to show up on the buf registry some time soon? the latest there is still at v1.178.0 🙏

[stephenh]

Hi @novascreen ! ts-proto used to push its own artifacts to the Buf registry, but as of ~ a year ago, I believe Buf controls all publishing of the community plugins like ts-proto.

@timosaikkonen does that sound right? Is there anything you can poke at, to get the latest versions pulling back into the Buf registry?

Thanks!