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

Generated TypeScript message class not compatible with generated message interface #837

Open
bhouser opened this issue Jun 19, 2017 · 4 comments · Fixed by Szpadel/protobuf.js#2

Comments

@bhouser
Copy link

bhouser commented Jun 19, 2017

protobuf.js version: 6.8.0

Problem:
Message class instances cannot be assigned to interface typed variable (or function parameter);

Generated TypeScript definitions.

    interface IToken {
        message?: google.protobuf.IAny;
    }

    class Token {
        constructor(properties?: core.IToken);
        public message?: (google.protobuf.IAny|null);
        public static encode(message: core.IToken, writer?: $protobuf.Writer): $protobuf.Writer;
        public static encodeDelimited(message: core.IToken, writer?: $protobuf.Writer): $protobuf.Writer;
        public static decode(reader: ($protobuf.Reader|Uint8Array), length?: number): core.Token;
        public static decodeDelimited(reader: ($protobuf.Reader|Uint8Array)): core.Token;
        public static verify(message: { [k: string]: any }): (string|null);
    }

Code resulting in TypeScript Error:

const myEncodedToken = Token.encode(myToken);

TypeScript Error:

Error:(42, 37) TS2345:Argument of type 'Token' is not assignable to parameter of type 'IToken'.
  Types of property 'message' are incompatible.
    Type 'IAny | null | undefined' is not assignable to type 'IAny | undefined'.
      Type 'null' is not assignable to type 'IAny | undefined'.

A similar problem was fixed in response to this issue: #826. The types now correctly reflect that an instantiated message class can have undefined fields. But assignment to the interface (by calling Message.encode for example) is still impossible.

Example generated TypeScript interface which would fix the problem:

    interface IToken {
        message?: google.protobuf.IAny | null;
    }
Alexendoo added a commit to Alexendoo/protobuf.js that referenced this issue Jun 24, 2017
Messages with optional fields will now implement their corresponding
interfaces (IMessage)

fixes protobufjs#834
fixes protobufjs#837
Szpadel pushed a commit to Szpadel/protobuf.js that referenced this issue Jul 26, 2017
Messages with optional fields will now implement their corresponding
interfaces (IMessage)

fixes protobufjs#834
fixes protobufjs#837
@dcodeIO
Copy link
Member

dcodeIO commented Nov 24, 2017

Does this issue still exist in master? There have been a couple merges that might, or might not, prevent this. Otherwise, do you have an updated example causing the issue?

@bhouser
Copy link
Author

bhouser commented Nov 24, 2017

I haven't attempted with master. The earliest I'll be able to test it is Monday.

@mknapik
Copy link

mknapik commented Nov 27, 2017

I had the same issue in version 6.8.0. I've just upgraded to 6.8.1 and it seems it was fixed and type implements an interface.

@sherlock1982
Copy link

6.8.3 still the same issue.
In interface member is declared as (string|null|undefined) and in class it's string

lhchavez added a commit to replit/protocol that referenced this issue Dec 8, 2020
This change adds Flow declaration types so that we can check the types
of the generated code.

Notably:

- Adds a dependency to flowgen and calls it during compilation, which
  takes the `.d.ts` and creates a `.flow.js` based on it.
- Passes in the `--force-number` flag to pbjs, which causes it to not
  emit numbers as possibly being `Long`, since this confuses Flow. This
  only affects JSDoc declarations, not the generated code.
- Passes in the `--force-message` flag to pbjs. Otherwise, this emits
  invalid types: it tries to create code like this:

  ```TypeScript
  interface IMessage {
    field?: T|null;
  }

  class Message implements IMessage {
    field: T;  // notice the lack of optionality / nullness.
    ...
  }
  ```

  This violates the Liskov Substitution Principle, since `Message`
  cannot be passed to any place that accepts an `IMessage`, so
  TypeScript (correctly) rejects this as bogus. With this flag, the
  number of places that will accept an `IMessage` instead of a `Message`
  are limited to constructors, where we do want to have partial messages
  being passed in.

  This only affects JSDoc declarations, not the generated code.
- Removes all `@implements` annotations from the JSDoc, which takes care
  of the very last `IMessage`/`Message` confusion. This, together with
  the above element take care of
  protobufjs/protobuf.js#837
lhchavez added a commit to replit/protocol that referenced this issue Dec 14, 2020
This change adds Flow declaration types so that we can check the types
of the generated code.

Notably:

- Adds a dependency to flowgen and calls it during compilation, which
  takes the `.d.ts` and creates a `.flow.js` based on it.
- Passes in the `--force-number` flag to pbjs, which causes it to not
  emit numbers as possibly being `Long`, since this confuses Flow. This
  only affects JSDoc declarations, not the generated code.
- Passes in the `--force-message` flag to pbjs. Otherwise, this emits
  invalid types: it tries to create code like this:

  ```TypeScript
  interface IMessage {
    field?: T|null;
  }

  class Message implements IMessage {
    field: T;  // notice the lack of optionality / nullness.
    ...
  }
  ```

  This violates the Liskov Substitution Principle, since `Message`
  cannot be passed to any place that accepts an `IMessage`, so
  TypeScript (correctly) rejects this as bogus. With this flag, the
  number of places that will accept an `IMessage` instead of a `Message`
  are limited to constructors, where we do want to have partial messages
  being passed in.

  This only affects JSDoc declarations, not the generated code.
- Removes all `@implements` annotations from the JSDoc, which takes care
  of the very last `IMessage`/`Message` confusion. This, together with
  the above element take care of
  protobufjs/protobuf.js#837
- Forwards calls from `.create()` to `.fromObject()` to avoid breaking existing
  code (w.r.t. types).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants