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

Add types for custom tags on component prop #622

Closed
errnesto opened this issue Jul 28, 2021 · 17 comments
Closed

Add types for custom tags on component prop #622

errnesto opened this issue Jul 28, 2021 · 17 comments
Labels
🏗 area/tools This affects tooling ☂️ area/types This affects typings 👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on

Comments

@errnesto
Copy link

errnesto commented Jul 28, 2021

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…

@errnesto errnesto added 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Jul 28, 2021
@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jul 28, 2021

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
Copy link
Member

ChristianMurphy commented Jul 28, 2021

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 ChristianMurphy added the ☂️ area/types This affects typings label Jul 28, 2021
@ChristianMurphy
Copy link
Member

this also relates to #623 (review)

@wooorm
Copy link
Member

wooorm commented Jul 29, 2021

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
Copy link
Member

@errnesto friendly ping, any insights on:

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

?

@errnesto
Copy link
Author

errnesto commented Aug 3, 2021

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
Copy link
Member

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

@ChristianMurphy ChristianMurphy added 📚 area/docs This affects documentation 🙋 no/question This does not need any changes and removed 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Aug 3, 2021
@wooorm
Copy link
Member

wooorm commented Aug 21, 2021

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

@wooorm wooorm closed this as completed Aug 21, 2021
@github-actions

This comment has been minimized.

@wooorm wooorm added 🏗 area/tools This affects tooling 👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on and removed 🙋 no/question This does not need any changes 📚 area/docs This affects documentation labels Aug 21, 2021
@github-actions

This comment has been minimized.

@cassus
Copy link

cassus commented Aug 21, 2021

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
Copy link
Member

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:

  • custom elements are generally global
  • The content mapping pattern that IntrinsicElements uses is the same pattern used elsewhere in remark for extending content types
  • The components type is already fairly complex, adding another layer of abstraction to seems risky.
    Particularly being able to completely override it, which could easily get users into situations where a "fix" for a custom component ends up breaking everything else.

@SkySails
Copy link

SkySails commented Aug 25, 2021

@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
Copy link
Member

ChristianMurphy commented Aug 25, 2021

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

🤔 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
Copy link

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!

wooorm pushed a commit that referenced this issue Aug 26, 2021
Co-authored-by: Remco Haszing <remcohaszing@gmail.com>

Reviewed-by: Remco Haszing <remcohaszing@gmail.com>
Reviewed-by: Titus Wormer <tituswormer@gmail.com>

Related to GH-622.
Closes GH-638.
@ChristianMurphy
Copy link
Member

@SkySails
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏗 area/tools This affects tooling ☂️ area/types This affects typings 👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

5 participants