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: Implement RFC29 Processor Options #16806

Open
1 task
btmills opened this issue Jan 22, 2023 · 11 comments
Open
1 task

Change Request: Implement RFC29 Processor Options #16806

btmills opened this issue Jan 22, 2023 · 11 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@btmills
Copy link
Member

btmills commented Jan 22, 2023

ESLint version

v8.32.0

What problem do you want to solve?

We accepted RFC29 to make options available to processors. #12068 began the implementation, but it was never finished.

We discussed this in the 2023-01-12 TSC meeting as a solution to eslint/eslint-plugin-markdown#208 , and I'm opening this issue to track the implementation of the RFC.

What do you think is the correct solution?

RFC29 was written before flat config so only addressed how we'd do processor options in .eslintrc files. A new PR should adapt that approach for use with flat config.

Participation

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

Additional comments

No response

@btmills btmills added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Jan 22, 2023
@fasttime
Copy link
Member

Any progress on this? I could work on a new PR if nobody started already.

@snitin315
Copy link
Contributor

@fasttime feel free to work on this.

@fasttime fasttime self-assigned this Feb 21, 2023
@fasttime
Copy link
Member

I have now restarted to implement this feature in eslint#rfc29 and eslintrc#rfc29. I was able to rebase part of the changes of #12068 into the new eslint branch, and apply the rest manually to match the current repo structure. Expecting many more changes to be done.

@fasttime
Copy link
Member

I am unsure if it would be good to extend this feature to the flat config. With the flat config, a custom processor can be specified as an object rather than by package name. This already makes it easy to supply options to a processor using whatever API provided by the implementation.

Today, a flat config that passes options to a custom processor could look like this:

// eslint.config.js
import { FooBarProcessor } from 'eslint-plugin-foobar';

const processor = new FooBarProcessor({
    option1: 42,
    option2: true,
    ...moreOptions
});

export default [
    {
        files: ['src/**'],
        processor,
        rules: {
            semi: 'error'
        }
    }
];

Basically, the same pattern can also be used to pass options to formatters and parser, making it trivial to parameterize plugin functionality.

Adding support for processorOptions to the flat config would ensure compatibility with the .eslintrc-based config once the RFC is implemented, but I can't say if this change would be necessary. I am ready to do changes to the flat config as well if that is the consensus, but I would like to know what our stance is about this. Any opinions or relevant policies?

@mdjermanovic
Copy link
Member

Adding support for processorOptions to the flat config would ensure compatibility with the .eslintrc-based config once the RFC is implemented, but I can't say if this change would be necessary.

Just to clarify, .eslintrc config functionalities are frozen and the idea was to only add processorOptions to the flat config, so there are no compatibility concerns.

@fasttime
Copy link
Member

Just to clarify, .eslintrc config functionalities are frozen and the idea was to only add processorOptions to the flat config, so there are no compatibility concerns.

Thanks so much for the clarification @mdjermanovic. It wasn't obvious to me from the discussion so far that the changes would only apply to the flat config. After reading the transcript of the TSC meeting mentioned by @btmills this makes much more sense now (I should have done it from the start. Sorry...).

Just for me to understand: is there a specific reason why we would like to pass processor options through ESLint first and then to a processor, instead of asking users to configure a processor by themselves (see the example config in my previous comment)?
Specifically, will ESLint also use these processor options somehow as it does with parser and language options, or is it just to keep the mechanism passing options consistent with the current design?

@fasttime
Copy link
Member

I need to understand how the processor options in the flat config will be used and why they are necessary - see #16806 (comment) - before I can continue working on this, so waiting for feedback.

@JounQin
Copy link
Contributor

JounQin commented Aug 3, 2023

@fasttime

We are blocked by this feature in eslint-mdx

https://github.com/mdx-js/eslint-mdx/blob/1ca5afd1d6e40d021d33c6f3177447e38e7263db/packages/eslint-plugin-mdx/src/processors/remark.ts#L12

We need to determine whether or not to lint code blocks in .md or .mdx based on processorOptions.

Your proposed solution seems like a good workaround:

import { FooBarProcessor } from 'eslint-plugin-foobar';

const processor = new FooBarProcessor({
    option1: 42,
    option2: true,
    ...moreOptions
});

I'd like to try it in eslint-plugin-mdx

@fasttime
Copy link
Member

fasttime commented Aug 3, 2023

Thanks for the info @JounQin. Please, don't hesitate to contact me if you encounter any problems, as I'd be really interested to understand how well this solution works in practice.

It's been a while since the last update on this issue, but I'm still under the impression that the use cases that motivated RFC29 can be already handled without extending the config schema. That is to say, if you find an issue that is making it hard for you to work on your project without the proposed processorOptions feature, that would be a good point to invalidate my assumptions.

@JounQin
Copy link
Contributor

JounQin commented Aug 3, 2023

https://github.com/mdx-js/eslint-mdx#flat-config

@fasttime I think processorOptions is unnecessary for flat config now. But how to support it via flat config manually by plugin author can be documented.

@fasttime
Copy link
Member

fasttime commented Aug 3, 2023

https://github.com/mdx-js/eslint-mdx#flat-config

Well, that looks good 👍🏻

@fasttime I think processorOptions is unnecessary for flat config now. But how to support it via flat config manually by plugin author can be documented.

Documentation for new flat config features in plugins is still a big todo, there is a tracking issue for that: #17242. Feel free to add a comment there if you want to suggest a topic to be included in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Status: Feedback Needed
Development

No branches or pull requests

5 participants