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 notes on how to type props and components #2510

Merged
merged 7 commits into from
Jul 12, 2024
Merged

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Jul 8, 2024

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

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

Copy link

vercel bot commented Jul 8, 2024

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

Signed-off-by: Karl Horky <[email protected]>
Signed-off-by: Karl Horky <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2024

⚠️ 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 ✅

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

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wooorm
Copy link
Member

wooorm commented Jul 8, 2024

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
Copy link
Contributor Author

karlhorky commented Jul 8, 2024

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:

Kapture.2024-07-08.at.14.07.35.mp4

@wooorm
Copy link
Member

wooorm commented Jul 8, 2024

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

<Note type="info">
**Note**:
You don’t have to use this syntax with `@mdx-js/*` packages.
Or use it always.
If you’re using a bundler integration you can change between MDX and
markdown through the file extension (`.mdx` vs. `.md`).
Alternatively, `options.format` can be used.
</Note>
, 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.

Signed-off-by: Karl Horky <[email protected]>
@karlhorky
Copy link
Contributor Author

karlhorky commented Jul 8, 2024

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

Signed-off-by: Karl Horky <[email protected]>
Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite good.

  • 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 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.

@wooorm
Copy link
Member

wooorm commented Jul 10, 2024

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.

Signed-off-by: Karl Horky <[email protected]>
@karlhorky
Copy link
Contributor Author

karlhorky commented Jul 10, 2024

  • 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:

Signed-off-by: Karl Horky <[email protected]>
@wooorm wooorm changed the title Add TypeScript Props Type section to Using MDX Add notes on how to type props and components Jul 12, 2024
@wooorm wooorm merged commit 95ba33e into mdx-js:main Jul 12, 2024
6 checks passed
@wooorm wooorm added 📚 area/docs This affects documentation 💪 phase/solved Post is done labels Jul 12, 2024
@wooorm
Copy link
Member

wooorm commented Jul 12, 2024

thanks!

@karlhorky karlhorky deleted the patch-2 branch July 12, 2024 10:01
@karlhorky
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

3 participants