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 (#154), AsObject type, add clear_ methods, fix map default values #157

Closed
wants to merge 17 commits into from

Conversation

Santalov
Copy link

@Santalov Santalov commented Jul 26, 2022

In short:

What exactly changed.

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 data argument of fromObject function became nullable, so it matches the constructor declaration.
The fromObject and constructor arguments are consistent in terms of nullability. To determine, if a field is nullable, the following function is used:

export function canBeCreatedWithoutValue(
  rootDescriptor: descriptor.FileDescriptorProto,
  fieldDescriptor: descriptor.FieldDescriptorProto,
) {
  return (
    isOptional(rootDescriptor, fieldDescriptor) ||
    // Both proto2 and proto3 don't track presence for maps and repeated fields.
    // https://github.com/protocolbuffers/protobuf/blob/main/docs/field_presence.md#presence-in-proto2-apis
    isMap(fieldDescriptor) ||
    isRepeated(fieldDescriptor)
  );
}

This also makes repeated and map fields nullable in constructor (they weren't).

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).

toObject returns default values for maps (there was a bug #146 (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.

Types of getters and setters match exactly. This is ok, cos when a field always has value (!canHaveNullValue), assigning
undefined to it makes no sense: the getter of the field will always return some non-null value, so the code bellow will print 0.

let x = undefined;
msg.int64 = x; // msg.int is a proto3 int64 field
x = msg.int64;
console.log(x); // will print '0', might be confusing

The implication is that you cannot assign undefined to a field, that always returns non-null value (if you are using strict:true). This is a problem when dealing with optional fields, so clear_* methods were introduced. The plugin generates them for all fields, that had has_* method generated. Those methods are described in the official doc https://github.com/protocolbuffers/protobuf/blob/main/docs/field_presence.md

Tsconfig options strict, noFallthroughCasesInSwitch, noImplicitReturns, noPropertyAccessFromIndexSignature, noUncheckedIndexedAccess, noUnusedParameters are enabled in all tests.

Options noFallthroughCasesInSwitch and noImplicitReturns required no changes.

Option noPropertyAccessFromIndexSignature required to change method access in RPC from property access to element access.

Option noUncheckedIndexedAccess required to add non-null assertion operator (!) when accessing elements of #one_of_decls, methods mentioned above, and when returning cases[case] in oneof method. Tests were also adjusted.

Option noUnusedParameters was covered by if statement, checking nullable data argument. Tests were also adjusted.

I skipped some previously planned options.

Option exactOptionalPropertyTypes is supported only by Typescript >= 4.4, so it was skipped.

Option noUnusedLocals conflicts with interfaces, declared for RPC, so it was skipped too.

Santalov added 17 commits July 26, 2022 20:48
Removed RecursivePartial<T>, instead introduced AsObjectPartial, which is used as fromObject method argument. Make getters, setters and AsObject typed according to canHaveNullValue predicate. Brought AsObjectPartial into line with constructor's declaration.
Change method access in RPC from property access to element access.
Add non-null assertion operators in places where an indexed access occurs.
return data && message when fromObject has no field assignments.
@Santalov
Copy link
Author

Santalov commented Jul 26, 2022

Oh, and there is an ambiguous change, previously discussed here

The fromObject argument became nullable and it confilcts with the name of the method suggesting there should be an object. But making it nullable makes fromObject more similar to the constructor.

@thesayyn , We need your decision. If the change needs to be reverted, I will revert a couple of commits.

@thesayyn thesayyn self-requested a review July 26, 2022 21:32
@thesayyn
Copy link
Owner

thesayyn commented Jul 26, 2022

Hey. I am swarmed this week but I'll try to take a proper look.

EDIT: it would help me a lot if you could split each of these changes into their own PR.

@Santalov
Copy link
Author

Hey. I am swarmed this week but I'll try to take a proper look.

EDIT: it would help me a lot if you could split each of these changes into their own PR.

No problem, I will split the PR.

@Santalov
Copy link
Author

Hey. I am swarmed this week but I'll try to take a proper look.

EDIT: it would help me a lot if you could split each of these changes into their own PR.

I have not figured out a way to make new PRs independent, cos there are merge conflicts in generated files (maybe we should add them to gitignore). So PRs need to be merged in the following order:

@Santalov
Copy link
Author

Closing this PR, it duplicates ones listed in the comment above

@Santalov Santalov closed this Aug 28, 2022
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