Allow serializing identical messages

[rideg]

This PR fixes the issues explained in issue/283 by checking the if the incoming value is a Message and its typeName equals to the field descriptor's typeName.

[CLAassistant]

All committers have signed the CLA.

[fubhy]

Hmm... I don't think this is the ideal approach to this. I'd suggest to instead implement a generic ttype guard on the MessageType instead and, in there, only loosely check that the given Mesasge has a getType function and that it returns the same message type (typeName comparison)

is(message: unknown): message is Message<ThisMessageType> {
  // Check that `message` has a `getType` function and that that returns
  // a `MessageType` with `typeName === this.typeName`
}

[fubhy]

... So here, you'd instead do:

if (type.is(value)) {
  return value;
}

Note: Not sure if the name is is a good choice, but you get the idea.

I am not a maintainer though... So :-)

[fubhy]

I just ran into this too while trying to stub out a slim Message object in a test.

Generally I agree that instanceof should be used carefully, especially in areas of a library that are part of the public interface. But I also wouldn't go down the path of adding too aggressive runtime duck type checking as in that isMessage function.

I think it's good enough to check whether there's a getType function and whether it returns an object with a typeName property that matches!

[rideg]

@fubhy Thanks for checking my PR. Great suggestions! I changed accordingly. I came with "conforms" as a method name. However I need some help because I had to change the generator code as well, but I don't know how to regenerate the well known protobuf files and now everything is broken due to the missing methods there.

[fubhy]

@fubhy Thanks for checking my PR. Great suggestions! I changed accordingly. I came with "conforms" as a method name. However I need some help because I had to change the generator code as well, but I don't know how to regenerate the well known protobuf files and now everything is broken due to the missing methods there.

Yeah that's a bit of a chicken-and-egg problem... The make routine depends on the build output of tsc to run the protoc generator. In turn, protoc-gen-es depends on @bufbuild/protobuf which hosts some of the code that's generated by protoc-gen-es. So once that package contains code that doesn't pass the typecheck, you get stuck ... Sadly, tsc doesn't have a --transpileOnly option (ignore type checking during build).

Not sure how the buf.build team handles that locally but I simply revert all local changes to ./packages/protobuf whenever I end up in this situation and then run make again.

@timostamm @smaye81 I think it would be good to configure tsconfig.json for development to have path aliases set up to the src of each package in the monorepo and then use a ts-node shebang for something like ./bin/protoc-gen-es.ts with ts-node configured to transpileOnly. Just for local development...

I've configured my plugin repo similarly FYI.

[rideg]

Ok, it seems I fixed all the issues. All tests pass and no there are no lint errors or warnings. The main changes are in the following files:

In protoc-gen-es:

• packages/protoc-gen-es/src/declaration.ts
• packages/protoc-gen-es/src/javascript.ts
• packages/protoc-gen-es/src/typescript.ts

In @bufbuild/protobuf:

• packages/protobuf/src/message-type.ts
• packages/protobuf/src/private/field-wrapper.ts
• packages/protobuf/src/private/message-type.ts
• packages/protobuf/src/private/util-common.ts
• packages/protobuf/src/private/util.ts

In protobuf-test:

• packages/protobuf-test/src/tobinary.test.ts

In the docs:

• docs/runtime_api.md

I think during the next release the minor version might need to be bumped due to the API change, but I guess that is not part of this PR.

Edit: @fubhy thanks for the suggestions!

[rideg]

I force re pushed all commits because I had the wrong git user.name and user.email set.

[timostamm]

Sandor, thank you for looking into this!

In the issue you put up, we take the wrong code path if instanceof fails, which causes a crash. This is very specific to our "unboxing" of the well-known wrapper types (docs). We should be more tolerant there for sure, this was an oversight.

In other cases where instanceof might fail, we fall back on a code path that simply creates a new instance. This is a pretty robust way to handle duplicate generated code.

The best way forward here is to fix the code paths that need to be more robust, and not add a type guard on MessageType.

[fubhy]

@timostamm Are you against adding such a type guard on the generated code altogether or just for the sake of solving the original issue? I'd definitely see use for this type guard (see example below):

const conforming = new Example();
const nonConforming = new OtherType();
Example.conforms(conforming); // true
Example.conforms(nonConforming); // false

@rideg Apologies for leading you down the wrong path here!

[timostamm]

I'm against a type guard in the generated code because it will only help in a very narrow band of use cases, where you have a class, but it might not have the identity you expect. For cases like this, it is also possible to override instanceof, for example:

Object.defineProperty(SomeMessage, Symbol.hasInstance, {
  value(instance: unknown) {
    return instance instanceof Message && instance.getType().typeName == SomeMessage.typeName;
  },
});

To be truly useful, fields must also be compared, otherwise it would give a false positive for a message with a removed field, for example. So far, I don't believe we need this complexity though. We can simply make our code paths a bit smarter to solve the original issue.

Apologies to both of you for not chiming in earlier.

[timostamm]

There is another kind of type guard that would work well in combination with the recursive Plain<T> or message interfaces as described in #230 (comment):

function isMessage<T extends Message<T>>(arg: unknown, type: MessageType<T>): arg is Plain<T>;

This could be a welcome addition for folks who want to deconstruct and construct messages without caring about classes. But this is also something best added as a standalone function instead of a method on MessageType, if only to keep it tree-shakeable.

By the way, protobuf-ts has them - see the manual. They were added because it only generates interfaces, not classes, and there is no equivalent to instanceof. They are also quite expensive to run, and can't distinguish between a google.protobuf.Duration and a google.protobuf.Timestamp.

[fubhy]

Ok I see where you are coming from. Thanks for clarifying!

[rideg]

Sure, thanks for the responses, reviews. I am going to close this PR as it is not necessary anymore.