Update types file to conditionally support className for the RichTextProps

[TatisLois]

Hey! Hope I'm contributing the right way to the project, if not ill make any needed changes. I also noticed a failing test that was unrelated to my changes and failing on a fresh pull. Should I open a separate issue for it?

This PR should fix issue #93 cc: @angeloashmore

When using the we want to support the prop className only when there is a corresponding Component prop.

I tried a few different approaches, this was a good learning experience. Ultimately using a discriminating-union netted me the results we were looking for.

Example screenshot.

Not sure if this PR will conflict with this Draft PR: https://github.com/prismicio/prismic-reactjs/pull/77. It's a year old so I did not want to branch from it.

[angeloashmore]

Hey @TatisLois, thanks for the PR! No worries about ignoring #77 since it is quite dated. We can always merge the changes later if we find a need to.

I think we may want to change how the new props type is being constructed. RichTextPropsWithComponent is assuming className is always accepted if a prop is given for Component. This may not be the case if Component does not accept a className prop, such as in the following example component.

type MyCompProps = {
  children?: React.ReactNode;
};

const MyComp = ({ children }: MyCompProps) => {
  return <div>{children}</div>;
};

In this case, we would be allowing a className prop on the Rich Text component, even if MyComp does not accept it.

<RichText Component={MyComp} className="will-not-be-used" render={myField} />

className should result in a type error here since MyComp does not support className.

What we can do instead is append Component's props to RichTextPropsCommon (renamed to RichTextProps below since there isn't a need to have a common type).

export type RichTextProps<Component extends React.ElementType> = {
  elements?: {};
  htmlSerializer?: HTMLSerializer<React.ReactNode>;
  linkResolver?: LinkResolver;
  render?: RichTextBlock[];
  renderAsText?: any;
  serializeHyperlink?: HTMLSerializer<React.ReactNode>;
  Component: Component;
} & React.ComponentProps<Component>;

In case you are unfamiliar with TypeScript Generics, a generic is like a TypeScript type parameter, similar to parameters for a function. You can give types parameters to determine the final output type.

Here, we create a generic named Component, which must be a React.ElementType (e.g. "div", "span", or a React component like typeof MyComp). We then intersect the existing Rich Text props using & with React.ComponentProps<Component>.

React.ComponentProps<Component> will be equivalent to the props for the component. If Component="div" is given to the Rich Text component, this would include any <div> prop, including className. If Component={MyComp} is used, this would include any <MyComp> prop, which does not include className.

The type for the RichText component needs to change to accomodate the Component generic.

export const RichText: <Component extends React.ElementType>(
  props: RichTextProps<Component>
) => (React.ReactElement<any, any> | null) & {
  propTypes?: React.WeakValidationMap<P>;
  contextTypes?: React.ValidationMap<any>;
  defaultProps?: Partial<P>;
  displayName?: "RichText";
  asText: (input: RichTextBlock[]) => string;
  render: (input: RichTextBlock[]) => React.ReactNode;
};

The extra properties (propTypes, contextTypes, etc.) are taken from React.FC. We cannot use React.FC anymore since we are using a generic. In general, React.FC is discouraged anyway since it automatically adds a children prop to the type, which the Rich Text component does not support.

Hopefully this makes sense! Are you able to update the PR to include these changes? If you want more clarity on any of this, please let me know and I can explain it further.

[TatisLois]

Thanks for the informative write-up @angeloashmore! I'm learning a lot here.

After reading your feedback and applying your recommendations to my branch I noticed that at the intersection & React.ComponentProps<Component> all of the user component props are passed into RichTextProps. Causing the unintended effect of not enforcing any of our types. I added a screenshot that shows that it now allows any props to be passed into RichTextProps. (Lines 24-26)

I'll SUDO explain what I think should happen next.

1. We want to maintain the intersection between the Component (all refer to it as C to minimize confusion) and RichTextProps so that we can have access to the C prop.
2. At the point where the C props are being intersected with RichTextProps, I would first check if C is an HTML element. If it is then allow an optional className prop on the consumer component.
3. If the C is a React component then we want to check if it has the prop ClassName. If so then allow an optional className prop on the consumer component.

I think that might get us to a good place, what do you think?

[angeloashmore]

Hey @TatisLois, yup, we definitely don't want to break the types because of this. Based on some experiments on my side, I can only get the prop type merging to work when the Component prop is a React component. Setting it to "div", for example, would result in the behavior you're describing (accepting any prop).

Your plan sounds like a good compromise, so let's go with that. It sounds like you may need to use TypeScript's Conditional Types to check if C is an HTML element or a React component. Feel free to do something simpler if you have another approach in mind though. 🙂

Thanks for your effort here!

[angeloashmore]

Hey @TatisLois, thanks again for taking a look at this issue and exploring some fixes.

@prismicio/react v2 was just published with a new <PrismicRichText> component. It does not accept a className prop since the component only returns a fragment. If you need a wrapping element with a specific class, just add an element around <PrismicRichText>.

I hope that works for your use case. Thanks!