management-sdk icon indicating copy to clipboard operation
management-sdk copied to clipboard

feat: Add components

Open MokumJ opened this issue 3 years ago • 4 comments

  • CRUD component
  • add/update component fields to model/component

MokumJ avatar Aug 16 '22 07:08 MokumJ

I went for an approach where if you give multiple components to the addComponentField it creates under the hood a UnionComponentField if you add just one component it creates a basic component. Not sure or this is an approach you like but felt convenient for me

MokumJ avatar Aug 16 '22 07:08 MokumJ

Hey @MokumJ 👋🏼 Thanks for creating the PR. I am sorry it took us time to review this.

Would it be possible if we can have separate methods for Component and ComponentUnion field? Because there can be scenarios where we only want a single component in ComponentUnion field.

Also I was facing some issues with type of the addComponentField method. I believe we should type it like so

  addComponentField(
    field: Omit<
      CreateComponentFieldArgs,
      "modelApiId" | "parentApiId" | "componentApiIds"
    >
  ): Model;

I have added modelApiId and parentApiId to Omit because we can get it from this.args.apiId. Consequently, the type of addComponentUnionField would be

  addComponentUnionField(
    field: Omit<
      CreateComponentFieldArgs,
      "modelApiId" | "parentApiId" | "componentApiId"
    >
  ): Model;

I am removing componentApiId because we only need componentApiIds.

What do you think about this?

rajatsharma avatar Sep 06 '22 10:09 rajatsharma

Hi @rajatsharma, glad to hear from you and great you are picking this up :) I agree with separating the component and ComponentUnion field. I guess its also more in line with previous use case of simplefield/unionfield. I see indeed those types should be omitted.

Are you picking up this points en add to the commit or should I have a look into it?

I am not sure when I would have time for this. Maybe next monday.

MokumJ avatar Sep 06 '22 13:09 MokumJ

@rajatsharma I updated the Pr to your suggestions. It seems adding a union component to a component field is not working. I think this is due to additions fixes that should be done to your management server as part of a batch migration service. Am I right?

MokumJ avatar Sep 12 '22 11:09 MokumJ