Support default instances on singular/required fields

[oliveryasuna]

I feel like this option should already exists, but I read through the README and could not find it. Please let me know if I am missing something.

Given a message:

message ErrorResponse {
  uint32 code = 1;
  string message = 2;
  optional string details = 3;
}

It generates:

export interface ErrorResponse {
  code: number;
  message: string;
  details?: string | undefined;
}

That is expected behavior.

However, if I have the following message:

message Test {
  message Inner {
    string value = 1;
  }
  message A {
    string value = 1;
  }
  message B {
    string value = 1;
  }

  Inner inner = 1;
  oneof value {
    A a = 2;
    B b = 3;
  }
}

It generates:

export interface Test {
  inner: TestInner | undefined;
  value?: { $case: "a"; a: TestA } | { $case: "b"; b: TestB } | undefined;
}

export interface TestInner {
  value: string;
}

export interface TestA {
  value: string;
}

export interface TestB {
  value: string;
}

Why is Test.inner defined with | undefined and why is Test.value optional and also defined with | undefined?

[Simon-Chenzw]

same request

my hacky fix:

type NoUndefinedField<T> = {
  [P in keyof T]-?: NoUndefinedField<NonNullable<T[P]>>;
};

https://stackoverflow.com/questions/53050011/remove-null-or-undefined-from-properties-of-a-type

[oliveryasuna]

@Simon-Chenzw I opened a PR #1066. Not sure if I covered everything.

[stephenh]

Hi @oliveryasuna , @Simon-Chenzw ts-proto's current behavior stems from two things: a) proto3's original "all message fields are always optional" behavior, and b) ts-proto's POJO-based approach.

In proto clients that use classs (like Java, C++, or even class keyword in JS/TS), it's possible for the Test.inner to be set to a "default instance" of TestInner, but then still have a test.hasInner() "hazzer" method provide the user will "field presence" detection, i.e. to ask if inner was actually sent over the wire or not.

Because ts-proto's messages are "just POJOs" (which is really the defining characteristic of this library, vs. other protobuf TypeScript libraries) and not classes, it's not clear how to implement this functionality.

I.e. @oliveryasuna in your PR, this method:

function createBaseUseStrictUndefined(): UseStrictUndefined {
  return { inner: undefined, value: undefined } as unknown as UseStrictUndefined;
}

Is creating inner as undefined, not a default instance, and so if the wire encoding did not have an explicit value for the inner field (which is allowed to happen), you've created a UseStrictUndefined.inner where the type says "this is always set" but the field is still actually undefined.

So, to implement this functionality, you would at least need to teach ts-proto to create "default instances" of TestInner (which itself might need to create "default instances" of its own message-typed fields).

With that implemented, we would then need to decide if it's possible to retain the "hazzer" behavior, i.e. since we don't have methods-on-the-instance, we might have to do Test.hasInner(testMessage) has a generated static method, but then what does it check? How would it tell if inner is "a default instance"?

Further, the "default instance"-ness of inner can change--i.e. once the user mutates the value, and does testMessage.inner.value = "asdf", then inner is no longer considered "a default instance", and so the Test.hasInner(testMessage) has to flip from false to true.

This seems tricky to do. I guess we could generate an TestInner.isDefaultMessage(testInner) that basically checked every field (recursing into message types if necessary) to ask "are each of these fields set to the default value"?

[stephenh]

Fwiw I renamed the title; originally it had said "for required fields", but technically your example was using "singular fields" (where singular are "fields that aren't otherwise marked as optional or required or repeated").

I.e. a true proto required field would use the required keyword, which would actually make your current PR more viable, because we could assume that the inner: undefined from the createBase method would surely/eventually be set over with "the real inner value", since we assume the wire encoding must have it (although even then it'd be good to check that early, and not accidentally return an ill-formed message to the caller).

[oliveryasuna]

@stephenh There is clearly more to think about here than I thought.

Here is my temporary workaround. I will just have another layer in between my proto/request and view model layers.

interface FieldMetadata {
  optional: boolean;
}

type PatchMessage<Message, Fields extends Record<Exclude<(keyof Message), '$type'>, FieldMetadata>> = {
  [K in (keyof Message)]: (
      K extends '$type'
          ? Message[K]
          : (
              Fields[K] extends FieldMetadata
                  ? (Fields[K]['optional'] extends true
                      ? (Message[K] | undefined)
                      : Exclude<Message[K], undefined>)
                  : Message[K]
              )
      );
};

[oliveryasuna]

@stephenh I think this is outside the scope of this project because it would break the pattern it implements. I will leave it to you to decide to close the issue/PR or not.

[oliveryasuna]

@stephenh I'm thinking that, although it breaks the rules of Proto 3, the option proposed in the PR would be useful.