mdx-js / mdx

Markdown for the component era
https://mdxjs.com
MIT License
17.76k stars 1.14k forks source link

Add notes on how to type props and components #2510

Closed karlhorky closed 4 months ago

karlhorky commented 4 months ago

Initial checklist

Description of changes

Add a section on the Using MDX docs page to show how to type the Props object in TypeScript with JSDoc, and the features this unlocks with MDX Analyzer.

I was unsure where to put visual assets, so I uploaded them to this PR description for now:

Screenshot 2024-07-08 at 12 33 11 copy

Screenshot 2024-07-08 at 12 45 09 copy

Screenshot 2024-07-08 at 12 55 34 copy

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mdx ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 10, 2024 5:27pm
codecov-commenter commented 4 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (908ff45) to head (8ea0ff7). Report is 39 commits behind head on main.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2510 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 23 23 Lines 2693 2712 +19 Branches 2 2 ========================================= + Hits 2693 2712 +19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wooorm commented 4 months ago

This isn’t an MDX thing. It’s something Remco came up with for VS Code users (with which I am not saying it’s bad; it’s just, an idea, there). There are no tools to type check MDX files. TypeScript does not really have plugins yet. I don‘t think we’ve had a discussion about whether this should exist and how. I feel like we’d have to discuss many many things before we add this on the MDX website. Especially in this guide. Getting started is where MDX analyzer and TypeScript are covered. But even then: why should it be in the TS section when this isn’t a TS thing? And why should it be in the VS Code section when it’s already in the readme of our VS code integration?

karlhorky commented 4 months ago

This isn’t an MDX thing. It’s something Remco came up with for VS Code users (with which I am not saying it’s bad; it’s just, an idea, there). There are no tools to type check MDX files.

Interesting, I would say that the MDX Analyzer mdx.checkMdx errors in the images above constitute a tool that type checks MDX files. Only in VS Code and only as an editor decoration, but it's still type checking.

Also, maybe I'm misunderstanding "This isn't an MDX thing", but in my perception, an official "MDX" VS Code extension published by the maintainers of MDX does appear to be an MDX thing:

Screenshot 2024-07-08 at 14 13 15

I would personally see all UX/DX surface area here also as a part of the MDX experience.

I feel like we’d have to discuss many many things before we add this on the MDX website. Especially in this guide. Getting started is where MDX analyzer and TypeScript are covered. But even then: why should it be in the TS section when this isn’t a TS thing? And why should it be in the VS Code section when it’s already in the readme of our VS code integration?

Alright, understood. Happy to be a part of that discussion or not.

My first thoughts, why I put it here:

  1. TypeScript documentation shouldn't be separate from JavaScript documentation, it should be an "extension" thereof, embedded in the topic - see as prior art the JS/TS code block switchers in many docs sites
  2. The topic of Props is first discussed in this section of Using MDX, so it seemed like the best place to put it.
  3. I was intuitively looking for such documentation in the MDX docs - documenting how to declare the types of an MDX file (even if this eventually changes) seems to be a good best practice to suggest to users, and (for me) lends a feeling of robustness to MDX

Visual space

If it's undesirable for this to take up as much visual space, it could be collapsible as the other TS section is:

https://github.com/mdx-js/mdx/assets/1935696/0aefa724-137f-4679-a61e-54379eb9ded6

wooorm commented 4 months ago

Also, maybe I'm misunderstanding "This isn't an MDX thing", but in my perception, an official "MDX" VS Code extension published by the maintainers of MDX does appear to be an MDX thing:

There’s a difference with when you put something on the MDX website for how everybody can use the MDX format with all MDX tools, versus when you put something in a readme for mdx-analyzer. It’s a “trojan horse” for adding support for Props and TypeScript in other tools. Elevating/blessing this should be discussed at depth first. When people start adding this to their files, we can’t change it later.

1. TypeScript documentation shouldn't be separate from JavaScript documentation, it should be an "extension" thereof, embedded in the topic - see as prior art the JS/TS code block switchers in many docs sites

It’s a bit of a high-level statement, so also one from me: I generally don‘t think the proprietary closed-governance language TypeScript needs to be mixed into Javascript.

2. The topic of Props is first discussed in this section of Using MDX, so it seemed like the best place to put it.

We can indeed mention how to type check props in the place where props are discussed. I agree that that is good. It is optional information though. These docs are meant to be read from top to bottom by everyone. I don‘t think we should provide such in-depth info about one particular toolchain (vscode + typescript + mdx) in the main flow of the document. Something like a note, a la https://github.com/mdx-js/mdx/blob/716ab3c8031d2663711daf864ffb816991f0719a/docs/docs/what-is-mdx.mdx#L75-L82, and then linking through to mdx-analyzer, seems more reasonable?

Some quick pseudo-text:

<Note type="info">
  **Note**:
  Users of `vscode-mdx` can add type checking of `props` with a JSDoc comment.
  See [`mdx-js/mdx-analyzer`][mdx-analyzer] for more info.
</Note>

…

<Note type="info">
  **Note**:
  Users of `vscode-mdx` can add type checking of provided and passed components
  with a JSDoc comment.
  See [`mdx-js/mdx-analyzer`][mdx-analyzer] for more info.
</Note>

…

[mdx-analyzer]: https://github.com/mdx-js/mdx-analyzer#use

3. […] documenting how to declare the types of an MDX file (even if this eventually changes) seems to be a good best practice to suggest to users, and (for me) lends a feeling of robustness to MDX

Sure, but a) the robustness you want (and we’d also like to have!) doesn’t exist in MDX-the-language and most MDX tools yet, only in mdx-analyzer, tsc doesn’t have plugins that can fail a CI, b) that seems more for https://mdxjs.com/docs/getting-started/#types. I believe since writing that, we have some interesting docs in types/mdx: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/eff7ed85c1a09706f9dcbb5bf3efcad5c50767b6/types/mdx/index.d.ts. So, there’s room to improve that section. Finally, perhaps “how to declare the types of an MDX file” can best be answered by a new guide: https://mdxjs.com/guides/.

If it's undesirable for this to take up as much visual space, it could be collapsible as the other TS section is:

Right, I had similar thoughts. I opted for a <Note> and a link through to mdx-analyzer. Does that pseudotext above seem acceptable? Or still a <details>? Feel free to riff off of it.

karlhorky commented 4 months ago

Elevating/blessing this should be discussed at depth first. When people start adding this to their files, we can’t change it later.

That makes sense to me 👍 Especially if it is not yet a decided syntax

Finally, perhaps “how to declare the types of an MDX file” can best be answered by a new guide: mdxjs.com/guides.

Interesting idea - should I open a small strawman issue for this?

I believe since writing that, we have some interesting docs in types/mdx: DefinitelyTyped/DefinitelyTyped@eff7ed8/types/mdx/index.d.ts. So, there’s room to improve that section.

Happy to make some small edits here too, with a few words of guidance as to what you would expect

Right, I had similar thoughts. I opted for a <Note> and a link through to mdx-analyzer. Does that pseudotext above seem acceptable? Or still a <details>? Feel free to riff off of it.

Yep, sounds like a good compromise + start 👍 I applied a version of the changes in:

My single edit: added the link to the MDX VS Code extension page

wooorm commented 4 months ago

Interesting idea - should I open a small strawman issue for this?

Happy to make some small edits here too, with a few words of guidance as to what you would expect

I think you as a user—especially with a background as a teacher—have great knowledge of what users and beginners might be missing. The problem space. So, I’d really appreciate it if you can post the problems you are experiencing. But I think the more solution space stuff is best answered by the maintainers.

karlhorky commented 4 months ago
  • one thing I wonder: should it go at the top or bottom? I was leaning to top. At the bottom it attaches to the next heading. Do you heave a reason for the tail end?

I chose / interpreted that you meant bottom because:

  1. Describing the topic seems higher priority to me than adding on, and I usually write in order of priority
  2. It's not necessary to get started
  3. It feels like an "extension" topic somehow - if you want to do more

But I can move to top if preferred.

  • I do think the 2 links are confusing. This isn’t Wikipedia. We don’t need to link every concept ever. Adding a link means people will open it and read it. You loose readers that way. This note is for people that already use the VS Code extension. Or, if not, and this piques their interest, the mdx-analyzer link leads them there. These links provide nothing and cost too much.

I tend towards over-linking rather than under-linking, and I think definitely for me as a user, I would want a link to know how to extend my experience. But I removed the link now:

wooorm commented 4 months ago

thanks!

karlhorky commented 4 months ago

Glad to help, thanks for the review and merge :)