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

Use of protobuf.Message<IMyMessage> incompatible with generated typescript classes #422

Open
jroper opened this issue Oct 19, 2022 · 1 comment
Labels
bug Something isn't working kalix-runtime Runtime and SDKs sub-team

Comments

@jroper
Copy link
Member

jroper commented Oct 19, 2022

When we generate typescript interfaces, we generate interfaces like this:

export declare namespace domain {
  type AssetLocation = proto.example.locations.IAssetLocation &
    protobuf.Message<proto.example.locations.IAssetLocation>;
  
  type CheckedIn = proto.example.assets.ICheckedIn &
    protobuf.Message<proto.example.assets.ICheckedIn>;
}

export declare namespace AssetsByLocation {
  type State = domain.AssetLocation;
  
  type Events = domain.CheckedIn;
  
  type UpdateHandlers = {
    UpdateLocation: (
      event: domain.CheckedIn,
      state: State | undefined,
      ctx: View.UpdateHandlerContext
    ) => State;
  };
}

So, the UpdateLocation callback has to return something that implements both proto.example.locations.IAssetLocation and protobuf.Message<proto.example.locations.IAssetLocation>. The problem is, the protobuf compiler doesn't generate anything that implements both of these, it generates this:

        /** Properties of an AssetLocation. */
        interface IAssetLocation {

            /** AssetLocation assetId */
            assetId?: (string|null);

            /** AssetLocation location */
            location?: (string|null);
        }

        /** Represents an AssetLocation. */
        class AssetLocation implements IAssetLocation {

            /**
             * Constructs a new AssetLocation.
             * @param [properties] Properties to set
             */
            constructor(properties?: example.locations.IAssetLocation);

            /** AssetLocation assetId. */
            public assetId: string;

            /** AssetLocation location. */
            public location: string;

So, in my UpdateLocation callback, I can instantiate an AssetLocation, but I can't return it, that will fail type checking. There's nothing that I can return. What we tell people to do is to do this:

const AssetLocation = view.lookupType("example.locations.AssetLocation");
const assetLocation = AssetLocation.create({assetId: "my-id"});

And that works, but the problem is that AssetLocation has a type of Message<{}>, so now we have lost type safety. I could do:

const AssetLocation = view.lookupType("example.locations.AssetLocation");
const assetLocation = AssetLocation.create({foo: "bar"});

That doesn't compile if I used the generated AssetLocation class, but does compile, and then fails silently at runtime, with the approach we force users to use.

The fact is, for these generated classes, we know exactly what type all the classes should be returning, so the metadata that we require to be there and extract from protobuf.Message shouldn't be necessary.

@jroper jroper added bug Something isn't working kalix-runtime Runtime and SDKs sub-team labels Oct 19, 2022
@pvlugter
Copy link
Member

Yes, one of the last awkward leftovers for the TypeScript SDK. It's because of the requirement to know the type for serialising states, which was only available in the reflective messages, not the statically generated ones. It can be fixed now that the generated static types have the type url in the new protobufjs release. Already tracked by #326.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working kalix-runtime Runtime and SDKs sub-team
Projects
None yet
Development

No branches or pull requests

2 participants