equinor / design-system

The Equinor design system
MIT License
118 stars 64 forks source link

Inconsistent Type definition for Components. E.g Typography #3634

Open yunarch opened 2 weeks ago

yunarch commented 2 weeks ago

Describe the bug

The TypographyProps definition is causing type extension issues due to its implementation as a type union of HTML node types, specifically HTMLElement and HTMLAnchorElement. This use of a type union introduces inconsistency at the type level, which is particularly evident when attempting to extend TypographyProps with an interface.

Attempted interface extension results in the TypeScript error:

An interface can only extend an object type or intersection of object types with statically known members.ts(2312)

I understand the goal, which is to achieve type safety for attributes such as href or rel related to HTMLAnchorElement. But there is a mix between type definitions and the usage of styled-components as property. This application creates a dichotomy at the type level that's not visually apparent, and it intermingles capabilities of styled-components without aligning them properly with the type definitions.

  1. EDS should allow extensions no matter if a user decides to use type or interface, it should not be forced to use one or another.
  2. As EDS is using styled-components, which allows to change dynamically the component HTML node element, it should also offers typesafety around that node. I should not have all the attributes no matter the html node i decide to use as that will bring a lot of confusion.

Steps to reproduce the bug

import type { TypographyProps } from '@equinor/eds-core-react';

interface Typography2Props extends TypographyProps {
/** New props ... */
}

Expected behavior

I should at least be able to extend the TypographyProps using interface.

Ideal solution, if i use as="a" then i will have autocomplete for the properties that are for an anchor element.

Possible solutions

An easy fix to allow extensions of the TypographyProps unsing interfaces is to instead of using type change it to interface which will force you to have double extension instead of a type union:

export interface TypographyProps extends extends HTMLAttributes<HTMLElement>, AnchorHTMLAttributes<HTMLAnchorElement>  {
  /** Typography variants, specifies which variant to use. */
  variant?: TypographyVariants
  /** Typography groups, specifies which group to use. */
  group?: TypographyGroups
  /** Bold text. */
  bold?: boolean
  /** Italic text. */
  italic?: boolean
  /** Link. */
  link?: boolean
  /** Typography colors. */
  // eslint-disable-next-line @typescript-eslint/no-redundant-type-constituents
  color?: ColorVariants | string
  /** Token. */
  token?: Partial<TypographyType>
  /** Number of lines. */
  lines?: number
}

Unsure if that change will produce other type errors.

A "more" correct fix as it will add inference arround the render engine (styled-components) is to allow autocomplete based on the as propert. A way to achive it is to create a wrapper around the forwardRef which will infer the types based on the as property.

A possible implementation, is just an example I think is quite correct but im using some deprecated types, but to test the idea should be good (Also keep in mind react 19 will remove the need for forwardRef, which then this code will need some refactor):

import {
  forwardRef as forwardReactRef,
  type ComponentProps,
  type ComponentPropsWithoutRef,
  type ElementType,
  type ExoticComponent,
  type ForwardRefRenderFunction,
  type PropsWithoutRef,
  type ValidationMap,
  type WeakValidationMap,
} from "react";

type As = ElementType;

type PropsOf<T extends As> = ComponentPropsWithoutRef<T>;

type RightJoinProps<
  SourceProps extends object = {},
  OverrideProps extends object = {},
> = OmitCommonProps<SourceProps, keyof OverrideProps> & OverrideProps;

type OmitCommonProps<
  Target,
  OmitAdditionalProps extends keyof any = never,
> = Omit<
  Target,
  "transition" | "as" | "color" | "translate" | OmitAdditionalProps
> & {
  htmlTranslate?: "yes" | "no" | undefined;
};

// ! If you plan to use this implementation keep in mind that this type must be called "ForwardRefExoticComponent" or storybook will not autogenerate the argTypes
type ForwardRefExoticComponent<
  ET extends As,
  P extends object = {},
> = ExoticComponent<P> & {
  <AsComponent extends As = ET>(
    props: MergeWithAsProp<
      ComponentProps<ET>,
      ComponentProps<AsComponent>,
      P,
      AsComponent
    >
  ): JSX.Element;
  displayName?: string;
  defaultProps?: Partial<P>;
  propTypes?: WeakValidationMap<P>;
  contextTypes?: ValidationMap<any>;
  id?: string;
};

type MergeWithAsProp<
  CP extends object,
  AsProps extends object,
  AdditionalProps extends object = {},
  AsComponent extends As = As,
> = (
  | RightJoinProps<CP, AdditionalProps>
  | RightJoinProps<AsProps, AdditionalProps>
) & {
  as?: AsComponent;
};

/**
 * Creates a forward ref for a React functional component that adds the "as" property.
 *
 * @typeParam P - The Component props type.
 * @typeParam ET - The React.ElementType.
 *
 * @param component - The functional component.
 * @returns Forwarded React component.
 */
export function forwardRefWithAs<P extends object, ET extends As>(
  component: ForwardRefRenderFunction<
    any,
    RightJoinProps<PropsOf<ET>, P> & {
      as?: As;
    }
  >
) {
  return forwardReactRef(component) as ForwardRefExoticComponent<
    ET,
    PropsWithoutRef<P>
  >;
}

Usage:

const BaseTypography = styled.p``

interface TypographyProps extends HTMLAttributes<HTMLParagraphElement> {
  /* ... */
}

 const Typography= forwardRefWithAs<TypographyProps, "p">(function Typography({children, ...others}, ref) {
  return <BaseTypography {...others} ref={ref}>{children}</BaseTypography>
 })

 const Test = () => {
  return <Typography as="a" href="...">Test</Typography>
 }
....

Now You should have autocomplete for href only if you use as="a"

yunarch commented 2 weeks ago

Just an update for the easy solution, as what i wrote it was just the general idea but in terms of code if you copy paste will not work. Is not possible to extend from both HTMLAttributes<HTMLElement> and AnchorHTMLAttributes<HTMLAnchorElement> at the same time as there is properties with the same name but not identically on the type def.

A solution for that could be:

export interface TypographyProps
  extends HTMLAttributes<HTMLElement>,
    Omit<
      AnchorHTMLAttributes<HTMLAnchorElement>,
      keyof HTMLAttributes<HTMLElement>
    > {
  /** Typography variants, specifies which variant to use. */
  variant?: TypographyVariants;
  /** Typography groups, specifies which group to use. */
  group?: TypographyGroups;
  /** Bold text. */
  bold?: boolean;
  /** Italic text. */
  italic?: boolean;
  /** Link. */
  link?: boolean;
  /** Typography colors. */
  color?: ColorVariants | string;
  /** Token. */
  token?: Partial<TypographyType>;
  /** Number of lines. */
  lines?: number;
}

I'm omiting the props from AnchorHTMLAttributes<HTMLAnchorElement> that their key is already on HTMLAttributes<HTMLElement> because your forwardRef is based on HTMLElement but you will still have autocomplete for props like rel or href.