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

Change Request: per-rule processors #17724

Closed
1 task done
mmkal opened this issue Nov 6, 2023 · 4 comments
Closed
1 task done

Change Request: per-rule processors #17724

mmkal opened this issue Nov 6, 2023 · 4 comments
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@mmkal
Copy link

mmkal commented Nov 6, 2023

ESLint version

v8.53.0

What problem do you want to solve?

I want to be able to write a rule in my plugin which doesn't have to fight with someone else's plugin, even if their plugin writes a process for the same kind of file.

Specifically, eslint-plugin-markdown and eslint-plugin-codegen both define processors for .md files, and they're not compatible: mmkal/eslint-plugin-codegen#7. eslint/rfcs#3 #11552 are no real help, though, and arguably make the situation worse by putting the decision on the end user by telling them to pick a processor in their config.

What do you think is the correct solution?

Scope processors to just the rule or at the plugin that defines them. So, without knowing the internals/ real code go from this pseudocode:

const processor = getProcessor(filepath, config)
const processedFile = processor(fileapth)

rules.forEach(rule => {
  rule.lint(processedFile)
})

to this pseudocode:

rules.forEach(rule => {
  const processor = getProcessor(filepath, rule.plugin)
  const processedFile = processor(fileapth)

  rule.lint(processedFile)
})

That way it doesn't matter if eslint-plugin-codegen does something absolutely crazy with markdown files, any other plugin can do their own separate crazy thing, and they won't affect each other.

Participation

  • I am willing to submit a pull request for this change.

Additional comments

I checked the "I am willing to submit a pull request" checkbox, but I would probably need a pointer to where this change might happen and any pitfalls to watch out for, and maybe some reassurance that this is doable without a huge overhaul.

@mmkal mmkal added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint labels Nov 6, 2023
@nzakas
Copy link
Member

nzakas commented Nov 9, 2023

Can you explain more about the problem you're having? I'm having a hard time understanding why the choice of processor matters. The processor in eslint-plugin-markdown just splits a markdown file into pieces of JS, which are then linted. I'm not familiar with eslint-plugin-codegen, what exactly are you using that for and how is it incompatible with eslint-plugin-markdown?

We can't guarantee that all plugins will work well together, there's just too large of an ecosystem to make any such claims.

arguably make the situation worse by putting the decision on the end user by telling them to pick a processor in their config.

ESLint is built on the assumption that the developer is the only one who knows the intent of what they're trying to do, so this is exactly what we want to happen.

Scope processors to just the rule or at the plugin that defines them.

Unfortunately, this isn't realistic. We intentionally keep the relationship between parts of a plugin very loose to make it easy to mix and match just what you want.

@mmkal
Copy link
Author

mmkal commented Nov 9, 2023

I should clarify I'm mostly asking this as a plugin author (I am the maintainer of eslint-plugin-codegen).

@nzakas I created a repro here: https://github.com/mmkal/eslint-processors-repro

Can you explain more about the problem you're having? I'm having a hard time understanding why the choice of processor matters. The processor in eslint-plugin-markdown just splits a markdown file into pieces of JS, which are then linted. I'm not familiar with eslint-plugin-codegen, what exactly are you using that for and how is it incompatible with eslint-plugin-markdown?

It's explained a little in the repro, but basically, eslint-plugin-codegen has its own custom processor which essentially "comments out" all the markdown in a file, in order to make it valid "js" and lint the file based on <!-- codegen:start {preset: markdownTOC} --> type directives. eslint-plugin-markdown splits into pieces of JS as you say. It's possible that I could rewrite my processor, or ask eslint-plugin-markdown to rewrite theirs, so they're compatible. But this feels like the wrong solution.

We can't guarantee that all plugins will work well together, there's just too large of an ecosystem to make any such claims.

What I'm proposing is a change that means you don't have to - each plugin would take the source file, runs its own processor on it, and passes the processed output to its rules.

We intentionally keep the relationship between parts of a plugin very loose to make it easy to mix and match just what you want.

I'm not sure I follow what you mean by loose here. I was imagining it'd be possible to do that in a non-breaking way - in this example, let eslint-plugin-markdown define the default processor, but allow me to define an alternative processor for eslint-plugin-codegen's rules.

Maybe I'm misunderstanding something though, what would you suggest I do if I want users of eslint-plugin-codegen to also be able to use eslint-plugin-markdown?

@nzakas
Copy link
Member

nzakas commented Nov 10, 2023

Okay, I took a look at your repro and your docs, and I'm still not entirely sure what it is you're trying to accomplish. As I already said, we aren't going to implement what you're suggesting because it introduces too much complexity into the system. Processors are already complex and their design hasn't changed fundamentally since they were first introduced years ago. Specifying processors on a per-rule basis instead of a per-file basis would require a dramatic rewrite of our config system, and it's just not worth it.

It's possible that I could rewrite my processor, or ask eslint-plugin-markdown to rewrite theirs, so they're compatible. But this feels like the wrong solution.

I think rewriting your processor to work with eslint-plugin-markdown is the correct solution here. eslint-plugin-markdown has been around for a long time, and it's intentionally a simple processor. Given that you're just starting your project now, I'd say it's up you to design your solution in such a way that it will work with the incumbent solution.

@nzakas nzakas closed this as not planned Won't fix, can't repro, duplicate, stale Nov 10, 2023
@mmkal
Copy link
Author

mmkal commented Nov 10, 2023

I'm still not entirely sure what it is you're trying to accomplish

I just want to be able to use both plugins at the same time, on a single file. They serve distinct, complementary purposes and right now, users must choose one or the other. Which is bad.


Given that you're just starting your project now, I'd say it's up you to design your solution in such a way that it will work with the incumbent solution

Since I happen to be the author of one of them, I'll try to make mine compatible since it's smaller. Worth noting: I'm not just starting out, eslint-plugin-codegen is about four years old, and has a few thousand downloads a week. Far less than eslint-plugin-markdown, but people do use it! I wonder if this can become a docs request - the processor docs say "you can create custom processors..." but they should probably say "you can create custom processors, but you probably shouldn't until you've verified that there are no incumbent processors that will compete with yours, because users will have to pick only one. It's your responsibility to find those incumbents".

For my own edification, I might explore this repo a little to see if there's something I can hook into that sidesteps this problem somewhat. Is there a starting point you would recommend? Maybe a processor is not the right tool for eslint-plugin-codegen after all - and I'm a little worried that eslint-plugin-markdown just chunks into js. My plugin is interested in all the markdown, not just js.

mmkal added a commit to mmkal/eslint-plugin-codegen that referenced this issue Nov 21, 2023
fixes #7

adds eslint-plugin-markdown as a dependency and updates the processor to
extend `eslint-plugin-markdown/lib/processor`. It adds one more "file"
to the array produced by eslint-plugin-markdown, which is the fully
markdown text, commented out. The rule then skips any files where the
`context.physicalName` is not equal to `context.filename` (see
eslint/eslint#17768).

It seems like weird design by eslint that plugins need to worry about
each other's processors, but hopefully there aren't many/any other
markdown processors in plugins out there:
eslint/eslint#17724

---------

Co-authored-by: Misha Kaletsky <mmkal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Archived in project
Development

No branches or pull requests

2 participants