-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
Conversation
8a248c4
to
7f1ae1f
Compare
To fully support ts, do we need to introduce ts-eslint/parser for testing? |
I'll give that a try to be safe. |
3da0276
to
bf33546
Compare
This is ready. |
// ESLintUtils.RuleCreator(docsUrl)({ ... }) | ||
(node.callee.type === 'CallExpression' && node.callee.callee.type === 'MemberExpression' && node.callee.callee.object.type === 'Identifier' && node.callee.callee.property.type === 'Identifier') | ||
) | ||
) { |
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.
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.
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.
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:
- 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. - We're only linting ESLint plugins, and if needed, a user can choose to only enable the
eslint-plugin/rules
config for theirrules/
directory. - 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. - 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.
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.
well, it also makes sense!
@not-an-aardvark @bradzacher, love to see your thoughts here. 😄 |
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.
💯 LGTM, thanks!
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.
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> |
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.
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) :(
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.
@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.
Fixes #176.
Adds ability for us to lint rules written using several common TypeScript rule formats:
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
getRuleInfo
that is responsible for this new TypeScript support.Part of v4 release (#120).