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

Change element types to be a discriminated union of all the element types #137

Closed
JiriLojda opened this issue Feb 2, 2024 · 4 comments · Fixed by #144
Closed

Change element types to be a discriminated union of all the element types #137

JiriLojda opened this issue Feb 2, 2024 · 4 comments · Fixed by #144
Assignees
Labels
enhancement New feature or request

Comments

@JiriLojda
Copy link
Member

Motivation

There is a base element type (e.g. ContentTypeElements.IElementShared) and a set of specific element types (e.g. ContentTypeElements.IAssetElement) that extend from the base type.
In places where there can be any of the specific elements (e.g. in the builder when upserting a content type or in the elements property when a content type is loaded from MAPI), the type of all the elements is the base type. This means that when I want to perform logic on a specific subset of element types, I have to manually assert the type (as MySpecificType).
Example:

const renderElement = (element: ContentTypeElements.IElementShared) => {
  switch (element.type) {
    case "asset": {
      const typedElement = element as ContentTypeElements.IAssetElement;
      // here I can work with the typedElement as an asset element
      // but nothing prevents me from doing "as ContentTypeElements.IDateTimeElement" instead so its easy to make a mistake
    }
    // ... similarly for other element types
  }
};

It is not only pretty tedious to write all the as assertions, but it also exposes us to mistakes asserting a different type than what is expected based on the value in the type property. TypeScript cannot help us here because of the structure of the element types in the SDK. Since the specific element types and the base type have no link between them (the interface extends doesn't create a link between those two, it just adds all the properties from the base into the specific element) TypeScript has no way of knowing what type is associated with different values of the type property.

Proposed solution

Fortunately, we can change the structure of the element types so that TypeScript can infer the proper type of the element based on the type property. We can leverage a discriminated union type for this. As a union type, the generic element would look something like this:

export type Element = // IElementShared doesn't make much sense as a name, because it is not about sharing properties
  IAssetElement
  | IDateTimeElement
  | INumberElement
  // ... rest of the elements

// we can still have a base element to share properties and avoid writing them in all the elements
// but now it doesn't make sense to export it, because it is not something users would need to use, its just for internal reusability
interface IAssetElement extends BaseElement {
  type: "asset";
  // ...
}
type BaseElement = {
  // ...
};

With the type written like this, we don't need to manually assert the element type, because TypeScript is able to infer it so we are safe from this type of mistakes.
Example:

const renderElement = (element: ContentTypeElements.Element) => {
  switch(element.type) {
    case "asset": {
      // here the parameter "element" is already typed as ContentTypeElement.IAssetElement, because TypeScript knows that
      // when "element.type === "asset" then the type of element must be IAssetElement
    }
  }
};

The same also applies to APIs where the user has to supply a value of type ContentTypeElements.Element. When the user specifies the type property, TypeScript (and IntelliSense) will know the rest of the properties that need to be specified and their type. Because of that, there would be no need for the element builders.

//...
.withData({
  elements: [
    {
      type: "asset",
      // here I get IntelliSense suggestions for all the properties and their types
    },
  ],
});

This improvement applies to all element types except for the variant elements because, in the language variant, there is no type property that could determine the type of the element.

@Enngage
Copy link
Member

Enngage commented Mar 1, 2024

Closed by #142

@Enngage Enngage closed this as completed Mar 1, 2024
@JiriLojda
Copy link
Member Author

Hi @Enngage,

I checked the #142 and it doesn't seem to fix this issue. It fixes another (related, but different) issue.

@Enngage
Copy link
Member

Enngage commented Mar 5, 2024

@Nickm615 can you please have another look? Perhaps try the example that @JiriLojda provided in the description to see if the types match the desired outcome.

@Enngage Enngage added the enhancement New feature or request label Mar 15, 2024
@Enngage
Copy link
Member

Enngage commented Mar 21, 2024

I've tested this as per the issue details and it works now properly ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants