stephenh / ts-proto

An idiomatic protobuf generator for TypeScript
Apache License 2.0
2.16k stars 348 forks source link

FieldMask type mistake with TypeScript #847

Open fengpeng opened 1 year ago

fengpeng commented 1 year ago

Hi~ I am using FieldMask to encounter some problems

// google/protobuf/field_mask.proto
message FieldMask {
  repeated string paths = 1;
}

generated typeScript code:

export interface PatchMeta_Request {
  fieldMask: string[] | undefined;
}

This request will call its own fromJson:

{
  key: isSet(object.key) ? FileId.fromJSON(object.key) : undefined,
  // 
  fieldMask: isSet(object.fieldMask) ? FieldMask.unwrap(FieldMask.fromJSON(a)) : undefined,
  //
  meta: isSet(object.meta) ? FileMeta.fromJSON(object.meta) : undefined,
}

But FieldMask.fromJSON doesn't support string[]

fromJSON(object: any): FieldMask {
    return {
      paths: typeof (object) === "string"
        ? object.split(",").filter(Boolean)
        : Array.isArray(object?.paths)
        ? object.paths.map(String)
        : [],
    };
 }

So, I read the source code, maybe that's the problem?

// ts-proto/src/types.ts
export function valueTypeName(ctx: Context, typeName: string): Code | undefined {
....
  case ".google.protobuf.FieldMask":
      return ctx.options.useJsonWireFormat
        ? code`string`
        : ctx.options.useReadonlyTypes
        ? code`readonly string[]`
        : code`string[]`;  // to code`{paths: string[]}`?
...
}

Thanks!

fengpeng commented 1 year ago

oh, I made a mistake. It's better to start with fromObject

  const canonicalFromJson: { [key: string]: { [field: string]: (from: string) => Code } } = {
    ["google.protobuf.FieldMask"]: {
      paths: (from: string) => code`typeof(${from}) === 'string'
        ? ${from}.split(",").filter(Boolean)
        : Array.isArray(${from})
        ? ${from}
        : Array.isArray(${from}?.paths)
        ? ${from}.paths.map(String)
        : []`,
    },
  };
stephenh commented 1 year ago

Hi @fengpeng ! Thanks for digging into the source... Fwiw I can't tell if someone is still wrong, or you figured it out. We have a unit test that shows FieldMask is working for our current set of tests:

https://github.com/stephenh/ts-proto/blob/main/integration/fieldmask/fieldmask-test.ts#L8

If you've found a bug, could you work on a reproduction, using that test as a starting point?

Thanks!

utrobin commented 4 months ago

Hi @fengpeng ! Thanks for digging into the source... Fwiw I can't tell if someone is still wrong, or you figured it out. We have a unit test that shows FieldMask is working for our current set of tests:

https://github.com/stephenh/ts-proto/blob/main/integration/fieldmask/fieldmask-test.ts#L8

If you've found a bug, could you work on a reproduction, using that test as a starting point?

Thanks!

I have a similar problem.

syntax = "proto3";

import "google/protobuf/field_mask.proto";

message UpdateRequest {
  google.protobuf.FieldMask update_mask = 1;
}

then

  protoc --plugin=node_modules/.bin/protoc-gen-ts_proto \
    -I=./proto/ \
    -I=./node_modules/google-proto-files/ \
    ./proto/test.proto \
    --ts_proto_out=src/test/

result

export interface UpdateRequest {
  updateMask: string[] | undefined;
}

but it was expected

export interface UpdateRequest {
  updateMask: FieldMask | undefined;
}

I want to clarify that the type for FieldMask was generated successfully, so the tests pass. However, this type is not being applied to the original message.

export interface FieldMask {
  /** The set of field mask paths. */
  paths: string[];
}