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

Support strict true, AsObject and AsObjectPartial types #159

Closed
wants to merge 7 commits into from

Conversation

Santalov
Copy link

@Santalov Santalov commented Jul 29, 2022

Related to #154
Tsconfig option strict is enabled in all tests. All tests extend base tsconfig.json in the test dir.
toObject method now returns AsObject type declared inside a message namespace.
fromObject now accepts AsObjectPartial type, that has as many nullable fields as possible, in contrast to AsObject.
These types are useful when dealing with plain objects, which is common in frontend applications.

The return type of the toObject method (AsObject) determines if the field is nullable by using this function:

export function canHaveNullValue(
  rootDescriptor: descriptor.FileDescriptorProto,
  fieldDescriptor: descriptor.FieldDescriptorProto,
): boolean {
  return (
    isOptional(rootDescriptor, fieldDescriptor) &&
    !(
      isNumber(fieldDescriptor) ||
      isEnum(fieldDescriptor) ||
      isRepeated(fieldDescriptor) ||
      isBytes(fieldDescriptor) ||
      isString(fieldDescriptor) ||
      isBoolean(fieldDescriptor) ||
      isMap(fieldDescriptor)
    )
    // isRequiredWithoutExplicitDefault is needed, cos
    // protoc-gen-ts does not throw an exception when deserializing a message
    // that does not have some required fields
    // (neither does Google's javascript implementation).
    // Thanks to @tomasz-szypenbejl-td for reference.
  ) || isRequiredWithoutExplicitDefault(rootDescriptor, fieldDescriptor);
  // Fields that are not optional and have explicit default return that default.
  // Fields that are optional and are not messages, i.e.:
  // (
  //   isNumber(fieldDescriptor) ||
  //   isEnum(fieldDescriptor) ||
  //   isRepeated(fieldDescriptor) ||
  //   isBytes(fieldDescriptor) ||
  //   isString(fieldDescriptor) ||
  //   isBoolean(fieldDescriptor) ||
  //   isMap(fieldDescriptor)
  // ) === true
  // return their default value (zero, [], new Unit8Array(), '', false, new Map())
}

These functions have been derived after some discussion in #146 (link to the comment).

Getter and setter types changed, but this will only affect the strict:true users of typescript.

The function above is also responsible for getter and setter type nullability.

@Santalov Santalov changed the title Support strict true Support strict true, AsObject and AsObjectPartial types Jul 29, 2022
@thesayyn
Copy link
Owner

Sorry for the weeks-taking review. I hope this is not blocking you.

I have a few concerns about this PR;

1- I am not a huge fan of the synthetic AsObject type. (i think it's a mistake made by others and we should avoid it at all cost.) It is easy to break people's code if we ever/when decide to remove or alter its name I'd be much more comfortable with inline types as people can't refer to it via import

2- Partial type. Suffers from the same problem with number one. Which also I think unnecessary. I believe something like Partial<ReturnType<MessageType.prototype.toObject>>. Omit could be used to omit properties that doesn't have to be partial.

Perhaps, the need for these subtypes signals that we have missing type information in our getters and setters?

@Santalov
Copy link
Author

Santalov commented Aug 17, 2022

1- I am not a huge fan of the synthetic AsObject type. (i think it's a mistake made by others and we should avoid it at all cost.) It is easy to break people's code if we ever/when decide to remove or alter its name I'd be much more comfortable with inline types as people can't refer to it via import

Hey, I understand your concerns, but I think we can avoid breaking code in the future by using aliases.

 export namespace MessageType {
        export type AsObject = {};
        export type NewAsObjectName = AsObject;
 }

Yeah, removal is a breaking change. And I see you are against the possibility of this. Let me explain why do we may need these subtypes.

In frontend applications it is common to use plain objects and some libraries even require that. For instance, Redux highly recommends using only plain objects, 58k starts on Github, as well as NgRx, 7.3k starts on Github.

So in frontend applications, we usually convert proto into a plain object just after receiving and convert back to proto just before sending.

Of course, we can use types like type MenuItemObj = ReturnType<typeof MenuItem.prototype.toObject>, but it introduces boilerplate and also for unknown reason introduces problems in Angular templates.

image

While type MenuItemObj = Parameters<typeof MenuItem.fromObject>[0] works fine, but it has most of the fields nullable.

image

Of course, the decision is for you, but I would be happy if there were AsObject types.

2- Partial type. Suffers from the same problem with number one. Which also I think unnecessary. I believe something like Partial<ReturnType<MessageType.prototype.toObject>>. Omit could be used to omit properties that doesn't have to be partial.

Yeah, that was my initial idea. I have introduced a RecursivePartial generic type. Partial is not enough because of submessages.

type RecursivePartial<T> = {
    [P in keyof T]?: 
        T[P] extends (infer U)[] ? RecursivePartial<U>[] :
        T[P] extends Uint8Array ? T[P] :
        T[P] extends object ? RecursivePartial<T[P]> :
        T[P];
};

But there was a problem with maps. There is no way to determine if the field is a map or a submessage on the type level and map type turns into {[key: string]: string | undefined} | undefined which causes strict:true tests to fail. And Omit can't help, it doesn't handle recursion.

Perhaps, the need for these subtypes signals that we have missing type information in our getters and setters?

I haven't faced such problems. There is only a mismatch of the type nullability with the actual value nullability, which is fixed in this PR.

@thesayyn
Copy link
Owner

Two things that I don't want to continue having. namespace and exporting more than what you have put into your proto

Of course, we can use types like type MenuItemObj = ReturnType<typeof MenuItem.prototype.toObject>, but it introduces boilerplate and also for unknown reason introduces problems in Angular templates.

what if when you do const m = proto.toObject() type interference should give you the correct result if we return . Would that work for you?

@Santalov
Copy link
Author

Santalov commented Aug 17, 2022

Two things that I don't want to continue having. namespace and exporting more than what you have put into your proto

Fair enough.

what if when you do const m = proto.toObject() type interference should give you the correct result if we return . Would that work for you?

No, I need a named type to use it for typing Angular component's input properties and function arguments.

// This is a class linked to the template referenced in my previous message
export class MenuItemCardComponent {
  @Input() menuItem!: MenuItemObj;
}

I agree to inline AsObject type, but I want the problem with Angular templates to be solved.
I believe I have just found a source of the error: toObject method implements one from abstract class Message,
where it is declared with return type {}.

export abstract class Message {
  // ... 
  abstract toObject(includeInstance?: boolean): {};
  // ...
}

When I rename method to be toObjectTyped the problem above disappears. The fromObject method doesn't have such problems, which suggests the problem is inheritance.

I suggest adding a new method, called toObjectTyped or asObject or something else, that is not inherited, with the following body.

 toObjectTyped() {
     return this.toObject();
 }

Maybe we can have inline types and a compiler option to generate and export types like ReturnType<typeof MenuItem.prototype.toObjectTyped>?

@thesayyn
Copy link
Owner

thesayyn commented Sep 24, 2022

Still couldn't spare time to think about this thoroughly.

Maybe we can have inline types and a compiler option to generate and export types like ReturnType<typeof MenuItem.prototype.toObjectTyped>

this is what we've been doing so far. I'd be fine with inline types until we solve it. the ideal solution would be fixing https://github.com/DefinitelyTyped/DefinitelyTyped/blob/3407f3a072abbab47a7ad706c15f69d6b7926577/types/google-protobuf/index.d.ts#L128 to be unknown.

@thesayyn thesayyn closed this Apr 6, 2023
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 this pull request may close these issues.

2 participants