diff --git a/designs/2024-per-rule-autofix-configuration/README.md b/designs/2024-per-rule-autofix-configuration/README.md new file mode 100644 index 00000000..1451cdcd --- /dev/null +++ b/designs/2024-per-rule-autofix-configuration/README.md @@ -0,0 +1,179 @@ +- Repo: +- Start Date: 2024-10-22 +- RFC PR: +- Authors: [Samuel Therrien](https://github.com/Samuel-Therrien-Beslogic) (aka [@Avasam](https://github.com/Avasam)) + +# Per-rule autofix configuration + +## Summary + + +This feature aims to make it possible to control autofixes through shareable configuration on a per-rule basis. + +## Motivation + + +Some rules provide autofixing, which is great, but can sometimes be broken or otherwise simply unwanted for various reasons. +Unsafe autofixes should be suggestions, and broken fixes should be reported, *but* ESLint is a large ecosystem where some very useful plugins are not always actively maintained. Even then, wanting to disable an autofix for project-specific or personal reasons could still happen. + +## Detailed Design + + + +Similar to how Ruff () does it, a top-level key to specify which rules to not autofix would be in my opinion the least disruptive and forward/backwards compatible. It should be overridable in per-file configurations, and picked up when extending a configuration. + +Concretely, it could look like this: + +```js +export default [ + { + disableAutofixes: { + // We don't want this to autofix, as a rule suddenly not failing should require human attention + "@eslint-community/eslint-comments/no-unused-disable": true, + }, + rules: { + '@eslint-community/eslint-comments/no-unused-disable': 'error', + } + }, + { + files: ["*.spec.js"], + disableAutofixes: { + // Let's pretend we want this to be autofixed in tests, for the sake of the RFC + "@eslint-community/eslint-comments/no-unused-disable": false, + }, + }, +] +``` + +The fix should still exist as a suggestion. Only autofixing (when running `eslint --fix` or editor action on save) should be disabled. + +This means removing the `LintMessage.fix` object into the `LintMessage.suggestions` array ([`LintMessage` API](https://eslint.org/docs/latest/integrate/nodejs-api#-lintmessage-type)) + +```js +const newSuggestion = { + desc: 'Apply the disabled autofix', + fix: lintMessage.fix, +} +if (!lintMessage.suggestions) { + lintMessage.suggestions = [newSuggestion] +} else { + lintMessage.suggestions.push(newSuggestion) +} +lintMessage.fix = undefined +``` + +The chosen key name `disableAutofixes` aims to remove the concern about "turning on" an autofix that doesn't exist. Disabling autofixes for a rule that doesn't have any or doesn't exist should be a no-op. Just like turning `off` a rule that doesn't exist. The reasoning being that this allows much more flexible shareable configurations. + +## Documentation + + +I think that "Configuring autofixes" or "Disabling autofixes" could be documented as a subsection of [Configuring Rules](https://eslint.org/docs/latest/use/configure/rules). Or as a section on the same level (between "Configuring Rules" and "Configuring Plugins") + +As a new top-level property added to configuration objects, `disableAutofixes` should be documented in [Configuration Files > Configuration Objects](https://eslint.org/docs/latest/use/configure/configuration-files#configuration-objects) section. Additionally we may want to add a note to [Custom Rules](https://eslint.org/docs/latest/extend/custom-rulesd) to mention that some autofixes will be converted automatically into suggestions when the new feature is used. + +## Drawbacks + + +A potential drawback I could see is that the configuration for autofixing a rule is not directly related with the rule itself. As a counter, I'd say this is already the case for plenty of rule-related settings, environment and parser configurations, etc. It's also less of a drawback than [Alternatives - Configure in the rule itself](#configure-in-the-rule-itself). + +## Backwards Compatibility Analysis + + +Given that this proposal adds a new optional configuration section, this feature should be fully backwards compatible. Users that don't want to use this feature should stay completely unaffected. (see [Alternatives - Configure in the rule itself](#configure-in-the-rule-itself)) + +## Alternatives + + + +### Configure in the rule itself + +Another approach I can think of is to encode that in the rule config itself. Something like `"my-plugin/my-rule": "[{severity: "error", autofix: False}, {...otherConfigs}]"` but it's harder to commit to such a change, and means that any config extension needs to reconfigure the rule correctly just to disable autofixing (which is already an issue when someone wants to set a pre-configured rule as warning for example) + +### Use of a 3rd-party plugin + + is a tool that exists to currently work around this limitation of ESLint, but it is not perfect. + +1. It is an extra third-party dependency, with its own potential maintenance issues (having to keep up with ESLint, separate dependencies that can fall out of date, obsolete, unsecure, etc.) +2. It may not work in all environments. For example, pre-commit.ci: +3. It may not work correctly with all third-party rules: + +## Open Questions + + +- Where exactly should the documentation go ? +- Where this needs to be implemented in code. Those familiar with ESLint's codebase are welcome to provide this information + +## Help Needed + + +My knowledge of ESLint's internals isn't that great. Whilst I think it's above the average user due to reading and configuring a lot, I haven't yet even learned how to write a plugin, and haven't migrated any project to ESLint 9 yet. +My free time both at work and personal, is currently also very limited (see how long it too me to just get to writing this RFC). +So I unfortunately don't think I can implement this feature myself, due to both a lack of time, personal motivation (I won't be able to use it for a while, but will push us towards ESLint 9 once implemented), and experience. + +## Frequently Asked Questions + + + +Q: Could `disableAutofixes` be an array of autofixes to disable? +A: `disableAutofixes` is a record to allow re-enabling autofixes in downstream configurations and on a per-file basis. We could allow a shorthand to `disableAutofixes` to accept an array of rules to disable the autofix for, but that would result in additional complexity on the implementation side with marginal benefits to the user. + +## Related Discussions + + +- +- +-