-
-
Notifications
You must be signed in to change notification settings - Fork 79
feat: definePlugin #132
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
base: main
Are you sure you want to change the base?
feat: definePlugin #132
Conversation
- Plugin names are often defined in at least four separate places: | ||
- `configs[string].name`, such as `"example/recommended"` | ||
- `configs[string].rules`, such as `{ "example/rule": "error" }` | ||
- `rules[string]`, such as `{ "example/rule": { ... } }` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When defining the rules objects for a plugin, it doesn't seem like the names are prefixed here. https://eslint.org/docs/latest/extend/plugins#configs-in-plugins
This would just be { "rule": { ... } }
and then the example/
would be set when the config registered this plugin as "example": pluginObject
.
It would be convenient for this utility to have some way of either automatically prefixing these rules when defining the config, or otherwise enforce that all of the rules exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that would be nice. It would be pretty ergonomic IMO if the new rules
could take in an array of rules objects.
The tricky point is, what would assign the rule name for the rules then? Rule meta
objects don't have a name field in ESLint core. typescript-eslint's RuleCreator
does but that's non-standard.
I would personally be in favor of:
- Adding an optional
meta.name: string
property defineConfig
allowing an array of rules rather than object iff each has a uniquemeta.name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's anything wrong with having to define the rules
as a map of "non-namespaced name" to rule object. It's when we have to add the namespace to specify the rules in the configs that this gets potentially messy.
I like the approach of having meta.docs.recommended
to determine the recommended
config, but perhaps that could go all the way to its conclusion: each rule could locally define (meta
) which configs it should be in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each rule could locally define (
meta
) which configs it should be in?
Yeah! That's what I'm going for with meta.docs.recommended
. The first version of this RFC just has true
support for a recommended
. From feedback in https://github.com/eslint/rfcs/pull/132/files#r2059307675 I'm expanding it now to also support valid severities such as "error"
and "warn"
. I suspect going further than that would get into tricky territory. Some plugins define specific configs as supersets of others, and having to describe all of those in each individual rule's meta
is a lot of duplicate & heavy work.
designs/2025-define-plugin/README.md
Outdated
- `recommended`: a config array of a single object containing: | ||
- `name`: per [configuration naming conventions](https://eslint.org/docs/latest/use/configure/configuration-files#configuration-naming-conventions) | ||
- `plugins`: an object keying `meta.name` to the plugin object | ||
- `rules`: all rules whose `meta.docs?.recommended === true`, with severity set to `"error"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand why it would be limited to just rules with error
severity. I know some really popular plugins that have rules with severity warn
in their recommended config (e.g. eslint-plugin-jest
). Idea: If meta.docs.recommended
was expanded to be boolean | 'warn' | 'error'
(and 1 | 2
?), then the logic could go something like
{
//...
rules: Object.fromEntries(
Object.entries(plugin.rules)
.filter(([, rule]) => rule.meta.docs?.recommended)
.map(([ruleName, rule]) => {
const recommended = rule.meta.docs.recommended;
if (["warn", "error"].includes(recommended)) {
return [ruleName, recommended];
} else {
return [ruleName, "error"];
}
}),
)
//...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind that rules in a config (therecommended
config) can be configured with a severity and options, neither of which is expressed in the rule's metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind, there are kind of two general paths plugins might want to go down:
- "Automatic": they don't want to be opinionated about specifics like error vs. warning. They just want it to be set up in a generally standard way.
- "Opinionated": they do care about things like error vs warning. They need tools to set up their opinions in a straightforward way, but are almost certainly going to need to customize things.
For "automatic" plugins, I think this is an opportunity to set one default behavior that works for the recommended starting plugin shape. Note that this is just the default; it's not a requirement. But it's a good way to enforce (what IMO is already) the default behavior of plugins that users generally expect.
For "opinionated" plugins, they don't have to go with these defaults - they can always configure with the stuff mentioned later on.
In other words: I think these these defaults need to make sense for the common case of plugins, but don't need to encapsulate every common customization (added rule options, non-error severity, etc.).
This stance is not very documented in the RFC right now...
neither of which is expressed in the rule's metadata.
But they could add it to the metadata, right? I think this is actually a not-rare practice. E.g. typescript-eslint does it for customizations to strict
configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For "automatic" plugins, I think this is an opportunity to set one default behavior that works for the recommended starting plugin shape. Note that this is just the default; it's not a requirement. But it's a good way to enforce (what IMO is already) the default behavior of plugins that users generally expect.
I definitely agree that having some default behavior that may not cover all use cases is valuable, but if a super common use case isn't covered, then its value is significantly diminished. Having rules with warn
severity in a recommended config is super common, and so I fear that if that isn't covered in some way by the default behavior, then there'd be a lot less people able to use the default behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @michaelfaith here - I don't think autogenerating configs where the severity is "error"
and cannot be overridden makes much sense. A concrete case: the @eslint/css
plugin has a recommended
config where only one rule is set to "warn"
. It's not a great experience if having one rule that needs to be set to "warn"
means I need to fully define the entire config.
It does seem like if we want to go down the route of fully-automatic configs then meta
needs a significant overhaul. To that end, it's worth thinking about whether or not a v1 of definePlugin()
needs this capability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My "never use warn
" bias is showing 😄. In the latest commit I expanded the default generated config design to allow either true
or any valid severity. So having meta.docs.recommended: 'warn'
will natively work to set that as the severity in the recommended
config. Is that a sufficiently scoped default behavior?
Linking to https://github.com/eslint/rfcs/pull/132/files#r2070777537: I suspect going further than that would get into tricky territory. Some plugins define specific configs as supersets of others, and having to describe all of those in each individual rule's meta is a lot of duplicate & heavy work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a good balance. 👍
In the latest commit...
Was that pushed? I'm not seeing that update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blagh sorry, just pushed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This RFC's scope intentionally does include a "rule creator" factory akin to [`@typescript-eslint/rule-creator`](https://typescript-eslint.io/developers/custom-rules/#rulecreator). | ||
Creating such a factory in ESLint could be useful but does not seem to be required for any of the problems `definePlugin` aims to solve. | ||
Should a "rule creator" factory be tackled separately? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an adjacent but separate conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work and research you put into this.
At a high-level, I can see the value of a definePlugin()
helper function that:
- provides type safety
- makes it easier to define configs
- enforces best practices (requiring
meta.name
,meta.version
, andmeta.namspace
I'm less convinced with some of the more ambitious parts of this proposal such as:
- lazy-loading rules
- autocreation of configs
By way of example, defineConfig()
s goals were primary to provide type safety and add an extends
key, so I'm wondering if maybe it makes sense to scale down definePlugin()
's MVP?
designs/2025-define-plugin/README.md
Outdated
- `recommended`: a config array of a single object containing: | ||
- `name`: per [configuration naming conventions](https://eslint.org/docs/latest/use/configure/configuration-files#configuration-naming-conventions) | ||
- `plugins`: an object keying `meta.name` to the plugin object | ||
- `rules`: all rules whose `meta.docs?.recommended === true`, with severity set to `"error"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @michaelfaith here - I don't think autogenerating configs where the severity is "error"
and cannot be overridden makes much sense. A concrete case: the @eslint/css
plugin has a recommended
config where only one rule is set to "warn"
. It's not a great experience if having one rule that needs to be set to "warn"
means I need to fully define the entire config.
It does seem like if we want to go down the route of fully-automatic configs then meta
needs a significant overhaul. To that end, it's worth thinking about whether or not a v1 of definePlugin()
needs this capability.
import packageData from "../package.json" with { type: "json" }; | ||
import { rules } from "./rules/index.js"; | ||
export const plugin = definePlugin({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A downside of this approach is that you can't tell just by looking at this code whether or not the plugin exports any configs.
designs/2025-define-plugin/README.md
Outdated
- `null | undefined`: to preserve that value | ||
- An array containing any number of elements, each either: | ||
- A rule object, in which case it will be turned into a namespaced rule based on the plugin's `meta.name` and the rule's key under the plugin's `rules` | ||
- An array containing a rule object and a severity to specify that rule as | ||
- An object with string keys and severity values, which will be directly merged into the resultant config `rules` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide some examples for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- A rule object ...: the
typescript-eslint
equivalent - An array containing a rule object ...: the
eslint-plugin-storybook
equivalent - An object with string keys and ...: the
eslint-plugin-markdown
equivalent
Are those what you're looking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More looking for actual code examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've come back to this comment a few times and have failed to parse each time. For "examples" / "actual code examples", the code blocks later on in the doc are what I would think to post. What info is being requested that's not being provided already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like examples right in this spot. The wording here is confusing and I can't really map each bullet point to any specific example below.
- An array containing a rule object and a severity to specify that rule as
Are you saying an array of arrays? It's unclear from the context. Imagine instead you put:
- An array containing a rule object and a severity to specify that rule as, for example: ...
And the example was right there. That's what I'm looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back at this, wow, yes, it was not clear. I overhauled the examples and explanations in f8e6acf.
Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
Co-authored-by: Kirk Waiblinger <53019676+kirkwaiblinger@users.noreply.github.com>
Added:
|
designs/2025-define-plugin/README.md
Outdated
`configs` is the only property `definePlugin` receives with a different shape than the output plugin object. | ||
|
||
- If `configs` is not provided, a default config is generated. | ||
- `configs` may be provided as a shape that does not include the plugin's name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not totally following this... Are we saying that if I provide
definePlugin({
meta,
rules,
configs: {
[`${pluginName}/recommended`]: { /* ... */ },
config1: { /* ... */ },
config2: { /* ... */ },
}
});
that all these configs should be transformed? Or that only the `${pluginName}/recommended`
one will be? Or maybe something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right, this is weirdly phrased and kind of wrong. It's the first one. Each individual config will be transformed. Edited:
- This part just says it's an object whose properties describe the configs to create.
- Generated Configs describes the key-value mapping in more detail
Is that more clear? I'm having a hard time describing this.
}); | ||
``` | ||
|
||
#### Generated Configs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this is just me, but I find the config generation abstraction a little daunting. Can it be simplified with an API like this?
import {
definePlugin,
inferRecommendedConfig,
pluginConfigFromRules,
} from "eslint/helpers";
export default definePlugin({
meta: {
name,
},
rules,
// plugin here is partially constructed, and lacks a `configs` field at this point
createConfigs(plugin) {
return {
...Object.fromEntries([
// registers plugin, infers recommended rules based on rules.foo.meta.recommended, sets
// name as `${pluginName}/recommended`. Same as the "default" config described in
// this RFC.
inferRecommendedConfig("recommended", plugin),
// creates `${pluginName}/someOtherConfig` that registers plugin and enables
// a few specified rules
pluginConfigFromRules("someOtherConfig", plugin, [rule1, rule2, rule3]),
]),
// for illustration purposes only. Normally you could use the above helpers
// only and simply `return Object.fromEntries(etc)`
myOwnHandwrittenConfig: {
/* whatever I want */
},
};
},
});
The idea I'm getting at here is
- The plugin author gets to construct a
configs
object that is fully WYSIWYG, which to me is really important. And I think not possible at all in the current design? - Helper functions could be provided for the obvious case of enabling a plugin and its recommended rules, and for the other use cases where a config generation format is provided here.
- Said helper functions will have descriptive names, rather than the shape of the provided object determining the behavior.
I guess the downside is that this approach doesn't guarantee that plugin.configs.foo.name === `${plugin.name}/foo`
if someone chooses not to use the provided helper(s)... Maybe that's still solvable?
FWIW I'm particularly averse to APIs that use the same field (configs
) both as an input and an output (plugin.configs
), but they aren't equal.
// given
const configs = { /* ... */ };
const plugin = definePlugin({
// ...
configs,
});
// This makes me feel :(
configs !== plugin.configs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplified
I find the current RFC design to be simpler personally, but maybe it's from having stared at it for too many hours. I'm personally not a fan of having to compose provided primitives the way inferRecommendedConfig
& co. would entail. It feels like a lot more work and functions to memorize to me. But I can empathize that the current "one definePlugin
to define them all" approach also has a lot of concepts.
fully WYSIWYG
I think it's ... almost conceptually WYSIWYG? The input format isn't exactly the same as output for rules
, but if you manually specify rules
then the conceptual "what configs exist with what rules" is the same.
doesn't guarantee that
plugin.configs.foo.name === `${plugin.name}/foo`
I don't follow, what do you mean by this? Are you looking at the input vs. output shape difference, or just the output itself, or...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't guarantee that
plugin.configs.foo.name === `${plugin.name}/foo`
I guess I'm saying, in an ideal world, definePlugin({ meta: { name: 'kirks-plugin' }, /* other fields */ })
will produce configs which look like
{
name: 'kirks-plugin/config-name',
// ^^^^^^^^^^^^ this is what I'm calling out
// rules, language options, etc...
}
for each of the entries of plugin.configs
. But if we allow fully manual, customizable configs (as I'd been suggesting), the author might not add the name
property to their fully hand-authored configs (or, worse, might put a name
other than kirks-plugin/config-name
).
I think it's ... almost conceptually WYSIWYG? The input format isn't exactly the same as output for
rules
, but if you manually specifyrules
then the conceptual "what configs exist with what rules" is the same.
it's true 🤷 . I'm the type of guy who would call the input field configTemplates
or something instead, rather than configs
, but it's intuitive enough.
Co-authored-by: Kirk Waiblinger <53019676+kirkwaiblinger@users.noreply.github.com>
Summary
Adds a
definePlugin
function to@eslint/plugin-kit
as a recommended way to group plugin configs, meta, and rules.Related Issues
definePlugin
function in@eslint/plugin-kit
rewrite#176