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

Breaking: Add support for TypeScript rules #197

Merged
merged 1 commit into from Oct 11, 2021

Conversation

bmish
Copy link
Member

@bmish bmish commented Sep 27, 2021

Fixes #176.

Adds ability for us to lint rules written using several common TypeScript rule formats:

// https://github.com/timdeschryver/eslint-plugin-ngrx
export default ESLintUtils.RuleCreator(docsUrl)<Options, MessageIds>({
  meta: {
    // ...
  }
})

// https://github.com/angular-eslint/angular-eslint
export default createESLintRule<Options, MessageIds>({
  meta: {
    // ...
  }
})

// https://github.com/typescript-eslint/typescript-eslint
export default util.createRule<Options, MessageIds>({
  meta: {
    // ...
  }
})

TypeScript rule detection

We look for files with a default export of any of the TypeScript rule helper structures from the above examples (e.g. util.createRule) including the presence of type parameters (<Options, MessageIds>). We don't actually verify util function names as those could be named anything.

This currently only supports TypeScript with ESM and not TypeScript with CJS. Based on my research of the ESLint plugin ecosystem, virtually nobody is using TypeScript with CJS. If people are using TypeScript with CJS to write rules, we could follow-up to include that.

Testing

  • I've added extensive tests of the util function getRuleInfo that is responsible for this new TypeScript support.
  • I've added tests to one rule as a sanity check to ensure it correctly handles TypeScript rules when run with the @typescript-eslint/parser.
  • I manually tested with the 3 TypeScript lint plugins from the example to verify that it was correctly picking up lint violations in real-world TypeScript rules.

Part of v4 release (#120).

@bmish bmish force-pushed the ts-rule-support branch 2 times, most recently from 8a248c4 to 7f1ae1f Compare September 27, 2021 14:05
@aladdin-add
Copy link
Contributor

To fully support ts, do we need to introduce ts-eslint/parser for testing?

@bmish
Copy link
Member Author

bmish commented Sep 27, 2021

To fully support ts, do we need to introduce ts-eslint/parser for testing?

I'll give that a try to be safe.

@bmish bmish mentioned this pull request Sep 27, 2021
@bmish bmish force-pushed the ts-rule-support branch 7 times, most recently from 3da0276 to bf33546 Compare October 3, 2021 19:38
@bmish bmish marked this pull request as ready for review October 3, 2021 20:07
@bmish
Copy link
Member Author

bmish commented Oct 3, 2021

This is ready.

@aladdin-add aladdin-add self-requested a review October 4, 2021 05:17
// ESLintUtils.RuleCreator(docsUrl)({ ... })
(node.callee.type === 'CallExpression' && node.callee.callee.type === 'MemberExpression' && node.callee.callee.object.type === 'Identifier' && node.callee.callee.property.type === 'Identifier')
)
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems likely to cause some false positives. I was wondering if we can support a new setting like createrFunc: ["createESLintRule"]. so it would not check other function calls - no false positives, and maybe faster.

Copy link
Member Author

@bmish bmish Oct 8, 2021

Choose a reason for hiding this comment

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

Requiring the user to have to configure a global .eslintrc.js setting for all these rules to work is a high barrier-to-entry that I would like to avoid. Perhaps as an optional setting for users with a custom util function, but not required.

Right now, I do believe the chance of false positives is actually very low, given:

  1. We're looking for a specific default-exported function call structure including parameterized types like this: export default [specific-function-call-structure]<Options, MessageIds>({ /* single parameter with object with relevant meta/create keys */ }). If we don't find those relevant keys inside this exact structure, we assume it's not a rule.
  2. We're only linting ESLint plugins, and if needed, a user can choose to only enable the eslint-plugin/rules config for their rules/ directory.
  3. Our getRuleInfo util function currently considers any default export with an object with relevant keys or a function definition to be a potential rule, so if anything, this TypeScript rule check has less chance of false positives than the existing function-style rule check.
  4. Given that we immediately check for this parameterized type structure <Options, MessageIds> and bail out if we don't see that, this new TypeScript rule check should not hurt performance at all either.

That said, I'm more than happy to try to make this TypeScript rule check more strict to further reduce the potential for false positives if we discover any additional import/type/other information we can cue off of. Let me know if you have ideas. But as is, I do believe that the existing checks are more-than-sufficient, and that we could treat any improvements as bug fixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, it also makes sense!

@aladdin-add
Copy link
Contributor

@not-an-aardvark @bradzacher, love to see your thoughts here. 😄

Copy link
Contributor

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

💯 LGTM, thanks!

@aladdin-add aladdin-add merged commit 382baa0 into eslint-community:master Oct 11, 2021
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

there will be some false-negatives (see comment) - but otherwise LGTM!
I have actually thought about doing this for a long while, I just never found the time!

Some of the rules will be rather valuable for easing maintenance burdens 😄

} else if (
node.type === 'CallExpression' &&
node.typeParameters &&
node.typeParameters.params.length === 2 && // Expecting: <Options, MessageIds>
Copy link
Member

Choose a reason for hiding this comment

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

note that it's not required that you declare type parameters.

In general - we are lazy and avoid declaring the type arguments if there are no options (example).

If there are no options then TS can infer [] as the options type, and it can infer the messageIds from the object literal definition.

Ideally as well you wouldn't ever need to declare two parameters... but that's a problem with TS (there's no way to tell TS "use this explicit generic and infer the second generic) :(

Copy link
Member Author

Choose a reason for hiding this comment

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

@bradzacher based on what you're telling me here, I'll remove the check for the type parameters to avoid false negatives. Let me know if there are any other common TS rule formats to support or other cues we can use to more-accurately detect TypeScript rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS support
3 participants