remarkjs / react-markdown

Markdown component for React
https://remarkjs.github.io/react-markdown/
MIT License
12.97k stars 867 forks source link

Add types for custom tags on component prop #622

Closed errnesto closed 3 years ago

errnesto commented 3 years ago

Subject of the issue

I think it is not possible to add components / renderers for custom tags like myCustomTag with typescript.

Problem

Lets say I'm using a plugin that creates custom tags e.g. myCustomTag and I want to render a custom component for it:

const components = { myCustomTag: ({node, ...props}) => <MyCustomComponent {...props} /> }

<ReactMarkdown
  remarkPlugins={[myPlugin]}
  components={components}
/>

Then I get a type error that myCustomTag is not allowed. This works fine if I set components to any.

Expected behavior

I should be able to add component renderers for custom tags without typescript complaining. It would be super cool if I could define custom tags with their respected properties…

ChristianMurphy commented 3 years ago

The types expect valid HTML, with TS 4.4 I could see custom elements like my-custom-tag being added with index signatures https://devblogs.microsoft.com/typescript/announcing-typescript-4-4-beta/#symbol-template-signatures. Something like

export interface CustomElements {
  [name: `${string}-${string}`]: NormalComponent
}

I'm not sure I see myCustomTag as being a part of react-markdown, it isn't valid markdown/HTML. If you're looking to mix JSX in markdown MDX/XDM may be a better fit https://github.com/wooorm/xdm

ChristianMurphy commented 3 years ago

Another approach, which I believe works today, would be to extend the IntrinsicElements type provided by the React types.

declare global {
    namespace JSX {
        // this merges with the existing intrinsic elements, adding 'my-custom-tag' and its props
        interface IntrinsicElements {
            'my-custom-tag': {'my-custom-attribute': string}
        }
    }
}

thoughts @wooorm on preferred approach? I'm learning towards extending IntrinsicElements similar to the pattern for extending AST nodes.

ChristianMurphy commented 3 years ago

this also relates to https://github.com/remarkjs/react-markdown/pull/623#pullrequestreview-717272026

wooorm commented 3 years ago

thoughts @wooorm on preferred approach?

I’d suggest to type JSX.IntrinsicElements.

I’d also want to hear more about what @errnesto is doing to get a custom component.

ChristianMurphy commented 3 years ago

@errnesto friendly ping, any insights on:

I’d also want to hear more about what @errnesto is doing to get a custom component.

?

errnesto commented 3 years ago

OH yeah sorry for the late reply :-)

We wanted to add something like https://github.com/elviswolcott/remark-admonitions to our markdown but have more controll over the generated html.

So we use https://github.com/remarkjs/remark-directive and add a simple 'plugin'.

The markdown looks e.g. like this:

:::note
Ab est nihil vero non sequi sapiente. Omnis doloremque temporibus
:::

This is the 'plugin'

  function customNotesPlugin() {
    return function transformer(tree: Node) {
      const visitor: Visitor<
        Node & { attributes: Record<string, string>; name: string }
      > = (node) => {
        if (node.name !== 'note') return
        const data = node.data || (node.data = {})
        data.hName = 'note'
        data.hProperties = { title: node.attributes.title ?? 'Note' }
      }
      visit(tree, ['containerDirective'], visitor)
    }
  }

And then this the ReactMakrdown instance:

<ReactMarkdown
  remarkPlugins={[
    remarkDirective,
    customNotesPlugin,
  ]}
  components={{
    note: ({ title, children }) => {
      return (
        <Note title={title as string}>{children}</Note>
      )
    },
  }}
  {...props}
/>

For me simply extending IntrinsicElements does solve the issue :-) (maybe this should be documented though?)

I could do what remark-math does and just return a div or span and then inspect this via a rehype plugin or add a function to render a div and check for a note className or prop… But this feels a bit messy to mee…

Also the docs say:

The keys in components are HTML equivalents for the things you write with markdown (such as h1 for # heading)†

† Normally, in markdown, those are: a, blockquote, code, em, h1, h2, h3, h4, h5, h6, hr, img, li, ol, p, pre, strong, and ul. With remark-gfm, you can also use: del, input, table, tbody, td, th, thead, and tr. Other remark or rehype plugins that add support for new constructs will also work with react-markdown.

ChristianMurphy commented 3 years ago

So we use https://github.com/remarkjs/remark-directive and add a simple 'plugin'.

Thanks for sharing!

For me simply extending IntrinsicElements does solve the issue (maybe this should be documented though?)

A PR improving documentation would be welcome!

could do what remark-math does and just return a div or span and then inspect this via a rehype plugin or add a function to render a div and check for a note className or prop… But this feels a bit messy to me

Up to you on the preferred way to render HTML, custom elements are also supported

Also the docs say:

The keys in components are HTML equivalents for the things you write with markdown

HTML supports custom elements (https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements), but they come with specific naming restrictions

Note that custom element names require a dash to be used in them (kebab-case); they can't be single words.

<my-note></my-note> and <custom-note></custom-note> are valid HTML, <note></note> is not valid HTML. If <note> were to fall through, with no custom renderer provided, React would throw a warning https://codesandbox.io/s/mystifying-jepsen-v04qk?file=/src/App.js

wooorm commented 3 years ago

Closing this as I feel this is something related more to how to deal with TypeScript, React, and JSX, rather than something about markdown and react-markdown. I’d say a link to the https://www.typescriptlang.org/docs/handbook/jsx.html#intrinsic-elements could be added somewhere, but I’m also fine with leaving this specialised knowledge out of the readme

github-actions[bot] commented 3 years ago

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

github-actions[bot] commented 3 years ago

Hi team! Could you describe why this has been marked as external?

Thanks, — bb

cassus commented 3 years ago

While understanding how to work with with custom tags and components I got an idea, a possible alternative resolution for these issues. (Is this the right place to put this?)

Currently the type of the components attribute is tied to the global IntrinsicElements type. And when that is too narrow we have the suggestion floating around to extend that global IntrinsicElements type. This feels a lot like mis-using global 'variables'.

Could we instead have a type parameter (generics) for ReactMarkdown defaulting to IntrinsicElements that could be overridden locally if needed -- without modifying any global types.

ChristianMurphy commented 3 years ago

This feels a lot like mis-using global 'variables'.

For the use case described in this PR it is not misuse. A custom element is being used, custom elements are global (https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements), TypeScript needs to be made aware to use them to be used in a TSX context anyway.

Could we instead have a type parameter (generics) for ReactMarkdown defaulting to IntrinsicElements that could be overridden locally if needed -- without modifying any global types.

Maybe? This would be good to open a new issue/discussion to go more in depth on. My initial reaction is probably not, for a few reasons:

SkySails commented 3 years ago

@ChristianMurphy While I agree that extending IntrinsicElements is not an anti-pattern, I am still having issues while using this approach.

I added this to my global type definition:

declare global {
  namespace JSX {
    interface IntrinsicElements {
      "full-bleed": { "src" : string };
    }
  }
}

but trying to register a component for my custom tag fails with this error:

Type '{ "full-bleed": ({ node, ...props }: { [x: string]: any; node: any; }) => JSX.Element; h1: (props: HeadingProps) => JSX.Element; h2: (props: HeadingProps) => JSX.Element; ... 12 more ...; shortcode: (props: any) => JSX.Element; }' is not assignable to type 'Partial<Omit<NormalComponents, keyof SpecialComponents> & SpecialComponents>'.
  Object literal may only specify known properties, and '"full-bleed"' does not exist in type 'Partial<Omit<NormalComponents, keyof SpecialComponents> & SpecialComponents>'.ts(2322)
ast-to-react.d.ts(1236, 3): The expected type comes from property 'components' which is declared here on type 'IntrinsicAttributes & Pick<Pick<ReactMarkdownOptions, "children" | "className" | keyof PluginOptions | keyof Options | keyof Options> & Pick<...> & Pick<...>, "children" | ... 9 more ... | keyof Options> & Partial<...> & Partial<...>'

The type definition in lib/ast-to-react.js seems to be correctly pointing towards the global IntrinsicElements type, but the library also contains a type definition file (ast-to-react.d) that contains what I presume are types that have been auto-generated from the above js file. In this case, it seems like the output from the generation process no longer implements the global IntrinsicElements, but instead iterates over it and generates one type per tagname:

type NormalComponents = {
    a: "a" | ((props: React.ClassAttributes<HTMLAnchorElement> & React.AnchorHTMLAttributes<HTMLAnchorElement> & ReactMarkdownProps) => ReactNode);
    abbr: "abbr" | ((props: React.ClassAttributes<HTMLElement> & React.HTMLAttributes<HTMLElement> & ReactMarkdownProps) => ReactNode);
    address: "address" | ((props: React.ClassAttributes<HTMLElement> & React.HTMLAttributes<HTMLElement> & ReactMarkdownProps) => ReactNode);
    area: "area" | ((props: React.ClassAttributes<HTMLAreaElement> & React.AreaHTMLAttributes<HTMLAreaElement> & ReactMarkdownProps) => ReactNode);
    article: "article" | ((props: React.ClassAttributes<HTMLElement> & React.HTMLAttributes<HTMLElement> & ReactMarkdownProps) => ReactNode);
    aside: "aside" | ((props: React.ClassAttributes<HTMLElement> & React.HTMLAttributes<HTMLElement> & ReactMarkdownProps) => ReactNode);
    ... 168 more ...;
    view: "view" | ((props: React.SVGProps<SVGViewElement> & ReactMarkdownProps) => ReactNode);
}

this seemingly results in me being unable to extend the type definitions for the components prop after the package has been compiled. Am I missing something obvious here?

ChristianMurphy commented 3 years ago

but the library also contains a type definition file (ast-to-react.d)

:thinking: hmm this seems to be an annoying quirk of JSDoc based typings (inlining intrinsics at build time https://unpkg.com/browse/react-markdown@7.0.0/lib/ast-to-react.d.ts). I've opened #638 to address this, it should compile to

export declare type NormalComponents = {
    [TagName in keyof JSX.IntrinsicElements]: TagName | ((props: JSX.IntrinsicElements[TagName] & ReactMarkdownProps) => ReactNode);
};

not inline the IntrinsicElements type.

SkySails commented 3 years ago

Great, that would confirm the issue I'm seeing. I will try out your PR and see if that fixes it!

Thank you for looking into this so fast!

ChristianMurphy commented 3 years ago

Give version 7.0.1+ a try https://github.com/remarkjs/react-markdown/blob/main/changelog.md#701---2021-08-26

SkySails commented 3 years ago

Sure, will do! I already tried with the PR and was successful in all I needed to do, so I'm sure that the new version will work as well!

Thanks again, much appreciated!