wooorm / xdm

Just a *really* good MDX compiler. No runtime. With esbuild, Rollup, and webpack plugins
http://wooorm.com/xdm/
MIT License
595 stars 18 forks source link

TypeScript for components prop? #25

Closed kentcdodds closed 3 years ago

kentcdodds commented 3 years ago

Any ideas on how to type the components prop? I've got https://github.com/kentcdodds/mdx-bundler/blob/dcae7709a15f3cc3d423784cd997d7e05e1d19d0/src/client.ts#L6 and need to add the components prop to that, but I'm not sure the best way to get a list of all possible component types. I could hand-write it, but wondered whether a list exists somewhere.

ChristianMurphy commented 3 years ago

Any ideas on how to type the components prop?

https://github.com/wooorm/xdm/blob/f9c3108bc86590b6f2ee20a5d93bc27107a8455d/lib/evaluate.js#L9-L10 is what XDM uses internally for the props. In your example it appears to be React specific, so it could be further narrowed to

interface ComponentMap {
  [name: string]: React.Component
}

interface MDXContentProps {
  [props: string]: unknown
  components?: ComponentMap
}

also note MDXContentProps is an index type, because arbitrary properties can also be passed to the content.

but I'm not sure the best way to get a list of all possible component types

Between custom node types and JSX tags, they could be just about anything, about the only restriction is the name should be text/string.

wooorm commented 3 years ago

The problem with “In your example it appears to be React specific, so it could be further narrowed to”, is that it’s not 100% true either.

As you can use for example <MyComponents.SomeComponent />, that is also picked up from components:


<Content components={{MyComponents: {SomeComponent: () => {}}}} />
wooorm commented 3 years ago

I’m not good at TS but something like this maybe?

interface ComponentMap {
  [name: string]: React.Component | ComponentMap
}
kentcdodds commented 3 years ago

Thanks! @Arcath fixed it for us in https://github.com/kentcdodds/mdx-bundler/pull/12

kentcdodds commented 3 years ago

Oh, actually one question:

because arbitrary properties can also be passed to the content.

What would be the purpose of this and how would those props be received? @Arcath, I think we didn't handle this case.

wooorm commented 3 years ago

I think @ChristianMurphy meant that arbitrary things can be passed in 'MDXContentProps'. (As in, they're given to the wrapper/layout). Not in 'MDXContentProps.components'

Arcath commented 3 years ago

I went with

{
  components?: Record<
    string,
    React.FunctionComponent | React.Component | string
  >
}

Which is working for my site and our tests.

ChristianMurphy commented 3 years ago

What would be the purpose of this and how would those props be received?

Beyond components, props can also be passed into a component. For example in this document

# <>{props.header}</>

calling the component with

{ "header": "Hello World" }

allows that props.header value to be set.

arbitrary things can be passed in 'MDXContentProps'. (As in, they're given to the wrapper/layout). Not in 'MDXContentProps.components'

Exactly

ChristianMurphy commented 3 years ago

React.FunctionComponent | React.Component

could be simplified with https://github.com/DefinitelyTyped/DefinitelyTyped/blob/0a800f58b8df8b26f8f9e7ad00c3fbd6ee31a602/types/react/index.d.ts#L78

{
   components?: Record<
     string,
     React.FunctionComponent | React.Component | string
  >
}

A good start, as noted above, not including the index would block typescript users from being able to pass props to the component. It also prevents nested component names, as @wooorm mentioned


Something like:

interface ComponentMap {
  [name: string]: React.ComponentType | string | ComponentMap
}

interface MDXContentProps {
  [props: string]: unknown
  components?: ComponentMap
}

could allow a more full breadth of MDX features to be used in TS

wooorm commented 3 years ago

One note to @ChristianMurphy's example: 'props' is not set. It can't be used in a heading. There is something called '_props', but it's not supposed to be used. That's passed through to the layout function, and so when that's a component can be used as normal props there. But it's not available in the content (unless you're using the hidden identifier, which you shouldn't)

wooorm commented 3 years ago

It might make sense to add that tho 🤔

ChristianMurphy commented 3 years ago

example: 'props' is not set

No? It's supported in MDX, was it intentionally removed in XDM?

It might make sense to add that tho :thinking:

It would be handy, there were quite a few MDX issues where folks needed to pass data into a component, props was the go-to answer, without it :man_shrugging: I'm not sure there are a lot of other options.

wooorm commented 3 years ago

I believe it used to be but was removed a long time ago 🤷‍♂️ somewhere soon after v1 maybe?

ChristianMurphy commented 3 years ago

was removed a long time ago ... somewhere soon after v1 maybe?

I'm not so sure about that, the playground is on 1.6.22 and supports props https://mdxjs.com/playground

Arcath commented 3 years ago

Something like:

interface ComponentMap {
  [name: string]: React.ComponentType | string | ComponentMap
}

interface MDXContentProps {
  [props: string]: unknown
  components?: ComponentMap
}

could allow a more full breadth of MDX features to be used in TS

Looks good to me. That works for my tests.

kentcdodds commented 3 years ago

Just pushed this improvement. Thank you!