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

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

Conversation

karlhorky
Copy link

Workaround for #454

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

Document workaround for Prettier with MDX + JSDoc props types

Copy link

changeset-bot bot commented Jul 8, 2024

⚠️ No Changeset found

Latest commit: e6aae5d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jul 8, 2024
Signed-off-by: Karl Horky <[email protected]>
@codecov-commenter
Copy link

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

Please upload report for BASE (main@a394ea8). Learn more about missing BASE report.

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #455   +/-   ##
=======================================
  Coverage        ?   91.43%           
=======================================
  Files           ?       10           
  Lines           ?     1600           
  Branches        ?        0           
=======================================
  Hits            ?     1463           
  Misses          ?      137           
  Partials        ?        0           

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

@karlhorky karlhorky changed the title Add prettier-ignore-start / prettier-ignore-end workaround Add prettier-ignore-start + prettier-ignore-end workaround Jul 8, 2024
Copy link
Member

@remcohaszing remcohaszing 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 not related to MDX analyzer, but I do think people will come looking for it here, so it’s helpful to document.

Since this is about compatibility with third party tooling, I think fits is better in the readme of the VSCode extension than that of the mono repo. Let’s add a Prettier section below the ESLint section.

README.md Outdated
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 v2+ JSDoc comments](https://github.com/prettier/prettier/issues/16457)):

```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

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.

Signed-off-by: Karl Horky <[email protected]>
Signed-off-by: Karl Horky <[email protected]>
@remcohaszing remcohaszing added 📚 area/docs This affects documentation 👶 semver/patch This is a backwards-compatible fix labels Jul 16, 2024
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/open Post is being triaged manually 👶 semver/patch This is a backwards-compatible fix
Development

Successfully merging this pull request may close these issues.

4 participants