eslint-plugin-eslint-plugin icon indicating copy to clipboard operation
eslint-plugin-eslint-plugin copied to clipboard

Rules do not work with named exports

Open Zamiell opened this issue 10 months ago • 4 comments

Hello, and thanks for the plugin.

I tried to use these rules on my plugin today, but none of them seem to work, meaning that none of the rules were triggered. I think this is because this plugin only looks for code matching export default foo or module.exports = foo.

My plugin does not use these and instead uses named exports. My plugin is a modern ESLint plugin written in TypeScript, and using default exports in TypeScript is considered an anti-pattern.

For example, here is what one of my rules looks like:

import { ESLintUtils } from "@typescript-eslint/utils";

const createRule = ESLintUtils.RuleCreator(
  (ruleName) =>
    `https://github.com/IsaacScript/isaacscript/blob/main/packages/eslint-plugin-isaacscript/docs/rules/${ruleName}.md`,
);

export type Options = [];
export type MessageIds = "noExplicitArray";

export const noExplicitArrayLoops = createRule<Options, MessageIds>({
  name: "no-explicit-array-loops",
  meta: {
    type: "problem",
    docs: {
      description: "Disallows explicit iteration for arrays",
      recommended: "recommended",
      requiresTypeChecking: true,
    },
    schema: [],
    messages: {
      noExplicitArray:
        'Explicit iteration over arrays is disallowed. (Get rid of the "Object.values()".)',
    },
    fixable: "code",
  },
  defaultOptions: [],
  create(context) {
    // Rule logic here.
  },
});

Thus, I propose that the "detect rule" logic of this plugin is updated in some way. If you can advise on what the correct solution is, I can submit a pull request, as I would love to use the rules that this project offers. Thank you.

Zamiell avatar Aug 26 '23 16:08 Zamiell

Thanks for your interest in using and contributing to our plugin!

Here's why we don't support your setup yet:

  1. While we support the ESLintUtils.RuleCreator helper when used directly, we don't yet support it when the function is stored in a variable. I would like to support this, but it would require additional static analysis (maybe our findVariableValue helper) to detect the variable and follow its usage, which can get a bit complicated, and we generally only bother with it for common use cases. PR welcome.
  2. We currently only look for default-exported rules because:
    1. At least in the traditional ESLint / CJS world, default exports made it easier to just do 'foo-rule-name': require('foo-rule-name') when exporting the rule objects for ESLint, without having to deal with transforming between rule names and variable names. So my impression is that default exports were pretty universal. In ESM/TypeScript, this benefit isn't as relevant, so perhaps named exports are fair-game / more common now. I see you have a script to generate the import/export index of rules that accommodates named exports, which is cool.
    2. Given the above assumption that almost everyone in the ESLint CJS world used default exports, ignoring named exports helped reduce false positives of detecting something that looked like a rule but wasn't.
    3. So overall, it sounds like supporting named exports is a change we could make. We can always use heuristics like limiting this support to ESM/TypeScript or even to the presence of certain known rule helpers to minimize the chance of false positives.

Here you can see the logic for how we detect rules and test case for the various setups we support:

https://github.com/eslint-community/eslint-plugin-eslint-plugin/blob/c308547450f20a6aa4d80c058fa8568dd10a1f6c/lib/utils.js#L320

https://github.com/eslint-community/eslint-plugin-eslint-plugin/blob/c308547450f20a6aa4d80c058fa8568dd10a1f6c/tests/lib/utils.js#L13

bmish avatar Aug 26 '23 23:08 bmish

@bmish Thanks for the reply. I spent some time attempting a PR, but unfortunately I think this task is a little over my head. :( Can I request that someone from your core team implements this feature? I think it will be important for the ecosystem going forward, especially as more ESLint plugins switch to ESM & the new config format.

Zamiell avatar Oct 04 '23 05:10 Zamiell

@Zamiell no worries, I'll take care of it.

aladdin-add avatar Oct 10 '23 07:10 aladdin-add

@aladdin-add Thanks for taking a look. Have you made any progress?

Zamiell avatar Dec 01 '23 15:12 Zamiell

@aladdin-add did you ever get around to this?

i use named exports in the various plugins i maintain, and have done for a long time now. so this plugin isn't of much use to me

if you don't have time, i can do the PR i'm sure. just let me know (although i'll probably go do it now anyway unless you already have one open)

edit: i had the time, so i went ahead and made the change in #449

43081j avatar Feb 20 '24 19:02 43081j