lightbend / kalix-javascript-sdk

JavaScript and TypeScript SDKs for Kalix
https://docs.kalix.io/javascript/index.html
Apache License 2.0
22 stars 21 forks source link

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

Open jroper opened 1 year ago

jroper commented 1 year ago

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.

pvlugter commented 1 year ago

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.