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

Feature/enable eslint-plugin-mdx #4326

Merged
merged 12 commits into from
Mar 22, 2021

Conversation

chenxsan
Copy link
Member

@chenxsan chenxsan commented Dec 20, 2020

We have some mdx files now, and it's likely that we will migrate all .md to .mdx, hence enabling eslint-plugin-mdx should help catching problems.

Thanks to @JounQin, all issues listed below are gone now.

There're some issues at the moment:
  1. Those code blocks in .mdx files won't get linted as eslint-plugin-mdx doesn't support it.
  2. vscode-mdx extension doesn't work as expected in my test.

@vercel
Copy link

vercel bot commented Dec 20, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/webpack-docs/webpack-js-org/4J62FJC7D7PnziVxqT86xh61jZDP
✅ Preview: https://webpack-js-org-git-fork-chenxsan-feature-enable-eslint-m-43ce20.vercel.app

@montogeek
Copy link
Member

montogeek commented Jan 5, 2021

and it's likely that we will migrate all .md to .mdx

Why?

@chenxsan
Copy link
Member Author

chenxsan commented Jan 5, 2021

and it's likely that we will migrate all .md to .mdx

Why?

Because the Badge component might be used in other markdown files. And we're maintaining two different configuration for md and mdx https://github.com/webpack/webpack.js.org/blob/master/webpack.common.js#L57, it's not necessary if mdx works for both cases.

@montogeek
Copy link
Member

Because the Badge component might be used in other markdown files. And we're maintaining two different configuration for md and mdx https://github.com/webpack/webpack.js.org/blob/master/webpack.common.js#L57, it's not necessary if mdx works for both cases.

Agree, but we shouldn't blindly switch everything when MDX features are only used in a few files of hundreds.
It will also impact the performance of the build.

@chenxsan
Copy link
Member Author

chenxsan commented Jan 5, 2021

Because the Badge component might be used in other markdown files. And we're maintaining two different configuration for md and mdx https://github.com/webpack/webpack.js.org/blob/master/webpack.common.js#L57, it's not necessary if mdx works for both cases.

Agree, but we shouldn't blindly switch everything when MDX features are only used in a few files of hundreds.
It will also impact the performance of the build.

Don't worry, I won't migrate all to mdx at once without any specific reason.

@JounQin
Copy link
Member

JounQin commented Mar 17, 2021

Hi, what do you mean by vscode-mdx extension doesn't work as expected in my test.?

Maybe related to mdx-js/mdx-analyzer#163?

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
@chenxsan
Copy link
Member Author

Hi, what do you mean by vscode-mdx extension doesn't work as expected in my test.?

Maybe related to mdx-js/vscode-mdx#163?

It won't highlight those errors:

image

here's a version of md:

image

.eslintrc.js Show resolved Hide resolved
@JounQin
Copy link
Member

JounQin commented Mar 17, 2021

It won't highlight those errors:

Yes, try to add eslint.validate as https://github.com/mdx-js/vscode-mdx/pull/163/files

@chenxsan
Copy link
Member Author

It won't highlight those errors:

Yes, try to add eslint.validate as https://github.com/mdx-js/vscode-mdx/pull/163/files

Cool, did fix the problem.

@chenxsan
Copy link
Member Author

@JounQin I just notice a warning on my local computer:

/Users/sam/Documents/githubRepos/webpack.js.org/src/components/NotificationBar/Notification.mdx
  1:94   warning  `"` can be escaped with `"`, `“`, `"`, `”`  mdx/no-unescaped-entities
  1:95   warning  `>` can be escaped with `>`                                   mdx/no-unescaped-entities
  1:111  warning  `>` can be escaped with `>`                                   mdx/no-unescaped-entities

Here's the content of Notification.mdx:

Webpack 5 has been officially released. Read our <a href="/blog/2020-10-10-webpack-5-release/">announcement</a>. Not ready yet? Read <a href="https://v4.webpack.js.org/">webpack 4 documentation here</a>.

Do you have any idea? I think it's valid syntax to use jsx inside markdown?

@JounQin
Copy link
Member

JounQin commented Mar 17, 2021

@chenxsan Please report it to eslint-mdx, I can reproduce it. It could be a regression.

You can wrap it into <></> as workaround temporarily. I'll take a look soon.

@chenxsan chenxsan merged commit 082db36 into webpack:master Mar 22, 2021
@chenxsan chenxsan deleted the feature/enable-eslint-mdx branch March 22, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants