Potentially unintended disparity between api and shapes module

[shimmerjs]

Hey guys : )

I've been writing higher level functions to make generation of common resources less boilerplatey. As a simple example (that doesn't make a ton of real world sense, but whatever) might look like a function which declares individual pieces (e.g., a v1.RoleRef) as parameters and declares that its return type will be a fully formed object (e.g., v1.RoleBinding). I have had plenty of success doing this, and in 9 out of 10 scenarios, I can do all of this using @jkcfg/kubernetes/api exports. However, for RBAC specifically (and maybe some others), I have noticed that I get type errors if I try to do that with @jkcfg/kubernetes/api:

export const roleBinding = (name: string, roleRef: api.rbac.v1.RoleRef, subjects: api.rbac.v1.Subject[]) => {
  return new api.rbac.v1.RoleBinding(name, { roleRef, subjects });
};

produces the error:

Type 'import("/Users/shimmerjs/code/common-blockchain-services/node_modules/@jkcfg/kubernetes/api").rbac.v1.RoleRef' is not assignable to type 'import("/Users/shimmerjs/code/common-blockchain-services/node_modules/@jkcfg/kubernetes/shapes").rbac.v1.RoleRef'.
  Types of property 'apiGroup' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.ts(2322)

The errors go away if I replace the parameter types with shapes.rbac.v1.*. This makes some sense to me, but since I only have to use @jkcfg/kubernetes/shapes for some resources, and not others, I was wondering if this is intentional and if so, what is the cause?

[squaremo]

Hmm let's see, I need to think this through from first principles. The definition of api.rbac.v1.RoleRef is

    export class RoleRef {
      public apiGroup?: string;
      public kind?: string;
      public name?: string;
    }

and in shapes,

    export interface RoleRef {
      apiGroup: string
      kind: string
      name: string
    }

All fields are marked optional in api, but some are mandatory interface getters in shapes -- those are only assignable from shapes to api, and not the other way around. You can assign the return value of a interface getter string to a field string | undefined, but you can't take the value of a field string | undefined and give it to an interface setter accepting string.

The constructor for api.rbac.v1.RoleBinding expects a shapes.rbac.v1.RoleRef, which has those mandatory fields, and it's getting an api.rbac.v1.RoleRef, with optional fields. So I can see now what it is complaining about.

The question is: what is the right way to use this? I believe the intention is that the constructor arguments can be api types or plain objects, so you can do

const rb = new RoleBinding('foo-binding', { roleRef: { name: 'foo-role' } })';

But since classes are also types, that would work without the shapes module, by having a constructor taking the type itself, and being careful with the generated constructors.

Would this work better for you @booninite ?

[shimmerjs]

Thanks for the speedy response as usual, I love chatting about jkcfg : )

Yes (assuming I am understanding correctly), what you are describing is how I am working with most of the classes exported by api today. I thought it was curious that for most Kube resources I was working with, the api and shapes types were compatible enough to use the api -- is this a side effect of something in the Kube swagger spec that the code is generated from? I've worked extensively with the types generated for the @kubernetes/client-node project and in my experience, they are all shaped like the classes exported by @jkcfg/kubernetes/api, with optional fields.

I would personally prefer being able to use the api types "end-to-end" in this way, but I have no issue using the shapes types. When I saw the discrepancy, I thought it was interesting enough to find out what the authors thought.

Cheers

[squaremo]

I thought it was curious that for most Kube resources I was working with, the api and shapes types were compatible enough to use the api -- is this a side effect of something in the Kube swagger spec that the code is generated from?

There's not that many required fields; possibly api.rbac.v1.RoleRef just happened to be the first type you used as an argument for which the fields differed in their optionality.

In any case, I think the alternative formulation will suit you well: you can just use api.* types for arguments, and TypeScript will enforce the required fields in the values supplied. If you want to allow some things to be optional which are not, you can use Partial<api.whatever.Class> as a type.

The shapes namespace would no longer be necessary (but might be deprecated for a minor version, so people's code doesn't break straight away).

I did a little code archaeology and it looks like the current arrangement is just a first pass on creating API types, albeit one that took a bunch of work. @dlespiau may be able to tell us about subtleties we have missed.

[dlespiau]

I think the idea was to be able to construct a valid object from various partial types. The partial types are useful when you don't want to specify a full valid object with mandatory field upfront but, say, merge some partials into a valid type.

That said shape.ts was done before I knew about some of the typescript type system. We could remove that file entirely and use Partial (actually one of the Deep/Recursive variant available on the Internet).

[squaremo]

The partial types are useful when you don't want to specify a full valid object with mandatory field upfront but, say, merge some partials into a valid type.

Something like:

function deploymentMeta(name: string): Partial<apps.v1.Deployment> {
  return new apps.v1.Deployment(name, {});
}

function deploymentSpec(containers: core.v1.ContainerSpec): Partial<apps.v1.Deployment> {
  return {
    spec: {
      template: {
        spec: { containers }
      }
    }
  };
}

function fullDeployment(metadata: Partial<apps.v1.Deployment>, spec: Partial<apps.v1.Deployment>): apps.v1.Deployment {
  return { metadata, spec };
}

(it's a rather artificial example, but it seems plausible that someone might want to make something like this, with the different functions in different modules, say).

Yeahp, that will still be possible using Partial<T> or a variant of it.

[squaremo]

I think I am persuaded to merge #65 and see how that goes, now 0.6.2 is released.

[shimmerjs]

Thanks for the responses, Partial<T> sounds like the correct solution to me as well. I think we can close this issue