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.

Sign up for GitHub

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 prettier-ignore-start + prettier-ignore-end workaround #455

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add prettier-ignore-start + prettier-ignore-end workaround #455

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9fb068
Add prettier-ignore-start / prettier-ignore-end workaround
karlhorky Jul 8, 2024
23a78ff
Fix formatting
karlhorky Jul 8, 2024
f3a07bd
Update README.md
karlhorky Jul 8, 2024
e6aae5d
Improve clarity
karlhorky Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,27 @@ For example:
# Hello {props.name}
```

#### JSDoc in MDX with Prettier

If you’re using Prettier, you’ll need to ignore any JSDoc comments until it is
supported (issues [MDX 3](https://github.com/prettier/prettier/issues/16457)
and
[MDX v2+ JSDoc comments](https://github.com/prettier/prettier/issues/16457)):
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to only link this issue: prettier/prettier#12209

Copy link
Author

Choose a reason for hiding this comment

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

done in:


```mdx
{/* prettier-ignore-start */}
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think using Prettier ignore comments is a good solution. Prettier doesn’t properly support MDX 3 yet overall. If you use this comment, another issue will pop up elsewhere. I would rather add the recommendation to add .mdx to .prettierignore.

Copy link
Member

Choose a reason for hiding this comment

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

I would also recommend adding *.mdx to .prettierignore 👍

Copy link
Author

@karlhorky karlhorky Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, I'm still going to try it out and stress test it for a while - I didn't see any other problems in a long document other than this issue with JSDoc

what about both?

  1. "since MDX 3 is not officially supported yet, we recommend ignoring *.mdx: <Prettier ignore config example>"
  2. "but another option is to ignore the parts that are not yet working: <existing example in PR>"

I think there will be at least some users who will want to stick with Prettier for most things, and add ignores for the parts that don't work, including these JSDoc comments

Copy link
Member

Choose a reason for hiding this comment

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

Prettier actively breaks good looking proper MDX (2+) files, not just expressions. MDX 2 got a ton of power and Prettier prevents people from using it.
I personally really really really would not recommend it, but I’ll leave this repo here to Remco.

Copy link
Member

Choose a reason for hiding this comment

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

I want this to be a small section explaining Prettier has issues, like the ESLint section. I don’t want to go into depth too much about which parts work or don’t work with Prettier (today). Something like:

## Prettier

Prettier supports MDX 1, but MDX 2 and 3 are [not officially supported](https://github.com/prettier/prettier/issues/12209) yet.
We recommend against formatting MDX files with Prettier.
To opt out, add the following line to your `.prettierignore` file:

```ignore
*.mdx
```

Copy link
Author

Choose a reason for hiding this comment

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

Ok, sounds mostly good

what do you think about my recommendation to add a second part after the codeblock with {/* prettier-ignore-start */}?

2. "but another option is to ignore the parts that are not yet working: "

my concern / reasoning for this is that I think it's not a very well-known option for users, and I think some users would rather ignore and still get some benefit from Prettier (eg. I like the formatting that it does apply to the other, supported sections)

Copy link
Member

Choose a reason for hiding this comment

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

As long as Prettier doesn’t properly support MDX 2/3, I really don’t think the MDX sources should recommend Prettier as a formatter. Of course you are free to have your own opinions and recommendations, but as the MDX team we should play on the safe side.

I think of this the same as using Prettier to support SVG/XML files by configuring Prettier to treat them as HTML. Sure, it’s largely compatible, but they’re really not the same. You will run into unexpected issues.


{/**
* @typedef Props
* @property {string} name
* Who to greet.
*/}

{/* prettier-ignore-end */}

# Hello {props.name}
```

#### `MDXProvidedComponents`

The special type `MDXProvidedComponents` is used to determine which components
Expand Down
Loading