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

Better support for multiple processors #11035

Closed
privatenumber opened this issue Oct 30, 2018 · 15 comments · Fixed by #11552 · May be fixed by Omrisnyk/npm-lockfiles#130
Closed

Better support for multiple processors #11035

privatenumber opened this issue Oct 30, 2018 · 15 comments · Fixed by #11552 · May be fixed by Omrisnyk/npm-lockfiles#130
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@privatenumber
Copy link
Sponsor

The version of ESLint you are using.
v5.8.0

The problem you want to solve.
I'm currently using two plugins to lint vue codeblocks in markdown files -- eslint-plugin-markdown and eslint-plugin-vue -- and they both have processors of their own that only apply to certain file extensions: eslint-plugin-markdown and eslint-plugin-vue. The problem is that the vue processor doesn't get applied to the Vue codeblocks in the markdown due to the file extension being md.

Your take on the correct solution to problem.
Wildcard extension support or the ability to add/mutate the effective filename of each src outputted by the preprocessor so that an extension can be appended (eg. the codeblock type).

Are you willing to submit a pull request to implement this change?
Yes

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Oct 30, 2018
@nzakas nzakas added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 30, 2018
@nzakas
Copy link
Member

nzakas commented Oct 30, 2018

Thanks for the issue. To summarize what I believe you're saying: you want to be able to lint JavaScript inside of a Vue template that ends with a .md extension. Is that correct? (If not, please feel free to correct me and don't bother reading the next bits because it won't be helpful.)

Assuming that is correct, I think you're running into what I think was a design flaw of the processor mechanism. Requiring plugins to specify a file extension to match processors to is a bit backward from the way the rest of ESLint works. (Chances are, I'm the one who designed it, so you can blame me.)

In order to accomplish this, I think what we should be looking at is a way to specify a processor in a config, probably as part of an overrides setting. That might require a change to how plugins expose processors so it's easier to reference from a config.

In any event, I think this is a problem worth solving.

@btmills what do you think?

@privatenumber
Copy link
Sponsor Author

Thank you @nzakas

I believe you've understood the problem accurately.

I also agree that specifying files by extension is not something the rest of ESlint does and that API might the root of the problem. However, I'm not sure if there's a better way to accurately identify a file type without excessive content parsing + identification.

As you suggested, a way for the user-end to override the extensions processors are applied to sounds like an appropriate solution to this.

@nzakas
Copy link
Member

nzakas commented Nov 1, 2018

What I had in mind was something like this in a config file:

module.exports = {

    overrides:  [
        {
            files: "*.vue.md",
            processor: "vue/markdown"
        }
    ]
};

It seems like that should be possible. We'd then need plugins to expose processors by name instead of by extension, but it seems like that wouldn't be a blocker.

@btmills
Copy link
Member

btmills commented Nov 5, 2018

@nzakas' proposal with overrides seems the most straightforward and easiest to implement as a default proposal.

One thing that occurred to me over the weekend is that code blocks in Markdown files effectively have extensions already:

# Docs

```js
// JS code here
```

```vue
// I don't know much about Vue, but this could be a snippet from a .vue file
```

What if the Markdown plugin sent those snippets back to ESLint as "here's a .js file, and here's a .vue file" (using "file" there liberally)? ESLint could then run back through and see that .js "files" have no more processing, and .vue "files" then get sent to the Vue processor.

Definitely an idea in the brainstorming stage, but I wanted to throw it out and see what you thought.

@mysticatea
Copy link
Member

mysticatea commented Nov 6, 2018

function preprocess(code: string, filename: string): string[]

This is the current interface.
We cannot chain the preprocess because the input type and the output type are different. I think that the enhancement that changes the output type solves this issue.

type VirtualFile = { code: string, filename: string }
function preprocess(code: string, filename: string): VirtualFile[] | string[]

Then we can chain the preprocess. For example:

<!-- example.md -->
# Docs

```js
console.log("hello")
```

```vue
<template><div>Hello</div></template>
```

```c
int main() {}
```
  1. eslint-plugin-markdown's processor returns

    [
        { "code": "console.log(\"hello\")", "filename": "example.md.js" },
        { "code": "<template><div>Hello</div></template>", "filename": "example.md.vue" },
        { "code": "int main() {}", "filename": "example.md.c" },
    ]
  2. eslint checks the virtual files whether it should lint or not by the filename. (by --ext option and input glob patterns?)

  3. eslint gives proper processors the virtual files. So eslint-plugin-vue can handle the <template><div>Hello</div></template>.

  4. Then, eslint gives rules the last code and filename.

  5. eslint gives processors the result messages to postprocess those in the reversed order of the preprocessing.

@privatenumber
Copy link
Sponsor Author

As suggested initially, I think a solution that mutates/appends to the effective filename or simply outputs an extension for processor matching would work too.

I think this will also be pretty interesting for Vue as you can specify blocks with different languages, and that could leverage this as well.

I was thinking overrides would be great in terms of giving user more control of what gets applied to what, but this only concerns processor application, and the two use-cases that I can think of are just Vue and Markdown. In both cases, the filetypes are already specified in the respective file-content on the user-end.

@ilyavolodin
Copy link
Member

I designed processors that way, because back when we added processors, we didn't have a concept of overrides and no way to specify file extensions in the config file itself. Also, the idea was that processors would require a lot less configuration that way. Processor itself would announce that it handles specific file types and ESLint would just send it all of the matching files. But I do agree that this approach provides less flexibility and file extension is not always a good indicator of the content of the file. It also doesn't really match what we are doing in other places in ESLint. So, based on that, I'm in support of redesigning processors API to be agnostic of the files that are being sent to it, and have ESLint configuration specify which files to send to processor using overrides (or in some cases, just saying that all files should be sent to processor).
@mysticatea proposal is interesting, my only concern is that now processor developers have to come up with a way to name arbitrary chunks of code that they extract from a given file. That doesn't seem like a huge issue, but I would also want to see how the postprocessor will be executed on a file that was chained through multiple processors.

@nzakas
Copy link
Member

nzakas commented Nov 7, 2018 via email

@nzakas nzakas added the needs design Important details about this change need to be discussed label Nov 19, 2018
@nzakas
Copy link
Member

nzakas commented Nov 19, 2018

The ESLint team has just created an RFC process for complicated changes that require designs. This issue was marked as "needs design" and so falls into this new RFC process. You can read more about the RFC process here:

https://github.com/eslint/rfcs/

I'll take a stab at creating an RFC for this.

@nzakas nzakas self-assigned this Nov 19, 2018
@not-an-aardvark
Copy link
Member

FYI there is already an RFC for this at eslint/rfcs#1.

@mysticatea
Copy link
Member

And I'm updating that for the new template.

@nzakas
Copy link
Member

nzakas commented Nov 20, 2018

@not-an-aardvark thanks for the heads up.

@mysticatea I hope you don't mind that I'm opening an RFC, too. I have a slightly different idea about how this could work.

@nzakas
Copy link
Member

nzakas commented Nov 20, 2018

Here is my proposal: eslint/rfcs#3

@nzakas nzakas added tsc agenda This issue will be discussed by ESLint's TSC at the next meeting and removed needs design Important details about this change need to be discussed labels Jan 15, 2019
@nzakas
Copy link
Member

nzakas commented Jan 15, 2019

The RFC for this change has been approved: https://github.com/eslint/rfcs/tree/master/designs/2018-processors-improvements

As this was the first merged RFC, I'd like to make doubly sure that the TSC accepts this by adding to the TSC agenda for this week.

TSC Summary: This proposal has a completed RFC explaining how to implement multiple processors for ESLint input files. There is no breaking change in this proposal. I'd like to review this because there was some confusion over the RFC process as to the timing of the merge (process is being discussed on a different issues now) and just want to ensure everyone is on board with this change.

TSC Question: Shall we accept this enhancement?

@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Jan 17, 2019
@btmills
Copy link
Member

btmills commented Jan 17, 2019

🎉 After today's TSC meeting, this is now accepted based on the RFC: https://github.com/eslint/rfcs/tree/master/designs/2018-processors-improvements

ilyavolodin pushed a commit that referenced this issue May 25, 2019
* New: multiple processors support (fixes #11035)

* improve coverage

* trivial fix

* improve coverage

* improve document

Co-Authored-By: Ilya Volodin <ivolodin@gmail.com>

* improve document

Co-Authored-By: Ilya Volodin <ivolodin@gmail.com>

* improve document

Co-Authored-By: Ilya Volodin <ivolodin@gmail.com>

* improve document

Co-Authored-By: Ilya Volodin <ivolodin@gmail.com>

* add note that `filterCodeBlock` option overrides the default behavior

* improve document about code block name

* extRegExp → extensionRegExp

* processor → processorName

* share unIndent function
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 22, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
6 participants