Skip to content

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review April 23, 2025 19:21
@fasttime fasttime added the Initial Commenting This RFC is in the initial feedback stage label Apr 24, 2025
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?

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.

Copy link
Member

@nzakas nzakas left a 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, and meta.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?

import packageData from "../package.json" with { type: "json" };
import { rules } from "./rules/index.js";
export const plugin = definePlugin({
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I like how this implicit approach nudges simpler plugins to default to the common default of just recommended. But I also see the advantage of simplifying the RFC + being more explicit. Do you / the review team have a preference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of auto-generating and wiring up configs, I just feel like that we need to make that explicit in the API somehow. Off the top of my head, something like autoConfig: true would at least give a hint that something is happening behind the scenes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me. I'll go ahead and remove config automation from this RFC and leave autoConfig: true for a followup. I think the FC discussion has gone on long enough that we'd be best trying to land the core pieces without these nice-to-haves. 👍

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Jun 16, 2025

I'm less convinced with some of the more ambitious parts of this proposal such as:

Added:

`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.

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?

Copy link
Contributor Author

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

cc4e515

Is that more clear? I'm having a hard time describing this.

});
```
#### Generated Configs
Copy link

@kirkwaiblinger kirkwaiblinger Jun 20, 2025

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

Copy link
Contributor Author

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...?

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 specify rules 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I brought up a similar concern in an earlier comment (which I can't find right now), that I am concerned about invisibly creating configs based on some heuristic. In general, I prefer APIs to show their work as much as possible.

I wonder if it might make sense to create a separate config helper that exists outside of definePlugin() to solve this problem? That way, we could still assign configs, but it would be clear that the meaning remains the same. Very rough sketch (do not take as 100% thought through):

export default definePlugin({

    configs: definePluginConfigs(rules)
});

or

export default definePlugin({

    configs: {
        recommended: definePluginConfig(rules)
    }
});

Copy link

@kirkwaiblinger kirkwaiblinger Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nzakas Sadly, it's not that easy though, since the configs need to reference the result of definePlugin() (which is one of the motivations for this RFC)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you mean definePluginConfig() would be a helper to create the template syntax, which would then be transformed again by definePlugin()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was just an API sketch, not a full proposal. The point is that we may be able to split these operations into separate chunks (or maybe not?).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure... but what problem is the sketch presenting a solution to?

Copy link
Member

@fasttime fasttime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work, and apologies for the delayed review. I've added some comments throughout. Given the length of the text, I may have missed something. Feel free to let me know if anything in my remarks seems off.

});
```
#### Generated Configs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I brought up a similar concern in an earlier comment (which I can't find right now), that I am concerned about invisibly creating configs based on some heuristic. In general, I prefer APIs to show their work as much as possible.

I wonder if it might make sense to create a separate config helper that exists outside of definePlugin() to solve this problem? That way, we could still assign configs, but it would be clear that the meaning remains the same. Very rough sketch (do not take as 100% thought through):

export default definePlugin({

    configs: definePluginConfigs(rules)
});

or

export default definePlugin({

    configs: {
        recommended: definePluginConfig(rules)
    }
});

[myRuleB, "error"],
[myRuleC, "warn"],
[myRuleD, ["error", "never"]],
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of introducing a new syntax here, especially as it doesn't offer any real advantage over what we already have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. @nzakas for this comment and others, I'm having a hard time parsing what the expected response is. This reads to me as general feedback, not a request for action. Are you requesting a change to the RFC? Or, how should I react to this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for being unclear. What I meant is that I don't like the idea of adding new syntax and would prefer not to do so, however, if you have a counter to my position then I'd like to understand your point of view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks. I also would prefer not to introduce new syntax. But I don't see a way around it that allows passing this style of "clean" object literal to definePlugin(). By "clean" I mean: there aren't any lazy functions, Object.defineProperty/get()s, or other constructs that add logic or delay evaluation.

The way I see the current proposal, it's a new wrapper ([myRuleB, ... ]) around the existing syntax ("error"). That feels like a small conceptual step to make.

But, is there an alternate proposal/syntax we could go with?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm having trouble understanding the benefit of this proposed format over the existing rules config format with rule-id: [severity, options]. To me, it looks like we're representing the exact same data in a slightly different way. It's possible I'm missing some nuance here, so can you walk me through it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is the rule-id bit. If this RFC sticks only with existing syntax, then the rule name would need to be referenced in >=2 places:

  • Once: as a key in the rules object ("my-rule-a": myRuleA,)
  • Once for each plugin.configs[string]: as a key in that config object's rules

In terms of code, sticking with existing syntax would commonly look like:

const examplePlugin = definePlugin({
  configs: {
    recommended: {
      "example/my-rule-a": "error",
      "example/my-rule-b": "error",
    },
  },
  meta: { name: "example" },
  rules: {
    "my-rule-a": myRuleA,
    "my-rule-b": myRuleB,
  },
});

Note also that the plugin's prefix, "example/", itself is added there once per rule. The plugin name and the rule names have become "magic strings" that need to be repeated.

The goal of this proposed syntax is to eliminate duplication of those strings: "example/", "my-rule-a", "my-rule-b". It instead uses the rules themselves as equivalents to keys:

const examplePlugin = definePlugin({
  configs: {
    recommended: [myRuleA, myRuleB],
  },
  meta: { name: "example" },
  rules: {
    "my-rule-a": myRuleA,
    "my-rule-b": myRuleB,
  },
});

Does that make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Initial Commenting This RFC is in the initial feedback stage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants