eslint / markdown

Lint JavaScript code blocks in Markdown documents
MIT License
406 stars 62 forks source link

[FR] option to ignore code blocks by meta string #208

Open Josh-Cena opened 2 years ago

Josh-Cena commented 2 years ago

Hi, we at MDN are considering adopting ESLint for our code examples.

We have got some initial success at fixing thousands of syntax errors / stylistic issues. However, we have some code blocks that intentionally demonstrate bad syntax:

```js example-bad
// You can't use a try...catch without braces
try doSomething();
catch (e) console.error(e);
```

We decided to not use eslint-skip anywhere (or at least as sparsely as possible), so that external contributors don't get confused. On top of that, we don't want to duplicate metadata: since example-bad is already used by the platform to render a code block with a cross, we think the linter should accommodate that convention.

Therefore, I want to propose a feature that ignores code blocks by meta string—currently, the parser takes advantage of glob patterns to differentiate code blocks, so adding a Markdown-specific option could be tricky. I don't have a solid API design on top of my head, but maybe we can add the meta string as part of the language extension (e.g. *.example-bad.js)?

lionralfs commented 2 years ago

I did some digging to figure out how to approach the proposed solution and stumbled upon this:

Apparently ESLint calls the preprocessor, which produces a list of blocks. ESLint then iterates over those blocks and (among other things) checks whether to lint each block or not based file path/extension. Here is the corresponding function, which blindly allows all .js files before consulting overrides[].files. It seems that this would not allow us to prevent *.example-bad.js from being linted.

Perhaps changing the preprocessor to not pass certain code blocks (based on meta info?) back to ESLint would be a viable alternative.

btmills commented 2 years ago

I'm excited you're thinking of using this for MDN! This is the same problem we encountered when we tried to lint our own docs: unlike most published code, so many of our code blocks are intentionally invalid.

Right now, there are two ways to avoid linting code blocks:

  1. <!-- eslint-skip --> comments, which you'd rather not use.
  2. Use a syntax that isn't targeted by any overrides.

There should be a third way by explicitly ignoring a virtual file path, but as @lionralfs discovered, that currently has a bug that is tracked in https://github.com/eslint/eslint/issues/15949.

I have three ideas:

  1. From https://github.com/mdn/mdn-community/discussions/158, it looks like you're able to define other syntaxes for highlighting that ESLint won't pick up and have already defined jssyntax for that purpose. Would using ```jssyntax example-bad in place of ```js example-bad be viable?
  2. This plugin has intentionally avoided doing anything with code block meta beyond the syntax because anything can go there, and using it may make this incompatible with other uses of the meta string. In your case, does example-bad only ever appear in isolation? If you're willing to fork the plugin, changing https://github.com/eslint/eslint-plugin-markdown/blob/0d4dbe8a50852516e14f656007c60e9e7a180b0a/lib/processor.js#L224 to if (node.lang && node.meta !== "example-bad") { would do the trick.
  3. Much less specifically, we could define a new API for this. Including the node.meta string in the block's file extension would be a breaking change. ESLint doesn't currently have a way to pass configuration to processors, so we'd be breaking new ground if we went that direction. If you're uninterested in <!-- eslint-skip --> comments, I'm guessing you wouldn't be a fan of <!-- eslint-expect-error --> (which could create an error if ESLint didn't find one) either?
Josh-Cena commented 2 years ago

RE: ```jssyntax example-bad—yep, that's one way. However, it does come with reduced DX—in my editor, if I use js, I get nice syntax highlighting. Usually the syntax is not broken enough to mess highlighting up, so I'll still value improved readability in this case.

If you're willing to fork the plugin

I'd certainly be willing to 😄 If there isn't a way to make it a core feature we can certainly maintain our fork. RE: "anything can go there", my goal with this FR is to make it an opt-in feature and users have to explicitly say "skip code with meta containing X". I think that's generalizable enough.

'm guessing you wouldn't be a fan of <!-- eslint-expect-error -->

Yeah, we'd prefer to not use any kind of comment but stick with meta string.

ESLint doesn't currently have a way to pass configuration to processors

That's unfortunate. I'll admit I don't know much about ESLint core APIs beyond developing rules... So if I understand correctly—the Markdown processor is not configurable in any way, and metadata format must be hardcoded (e.g. eslint-skip), right?

In this case, I think we can consider forking it since we can do other useful things—we have a lot of tricky code block usage that's hard to analyze.

btmills commented 1 year ago

Sorry for the long delay here.

When I wrote my previous comment about processor options being the ideal way to do this without forking, I had forgotten about an already-accepted RFC that would make options accessible to processors! There was a PR to implement it, but it was never finished.

I've opened a new issue to track the implementation of that RFC. If anyone's interested in picking up that work, we'd be able to add an option like ignoreBlockMetaPattern to this processor to tell it to ignore blocks based on a user-configurable meta string!