-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix(eslint-plugin): support JSX attrs in allowTypedFunctionExpressions #2
Conversation
75d92f7
to
dae81ed
Compare
* foo(() => 1) | ||
* ``` | ||
*/ | ||
function isFunctionArgument( |
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.
Moved to be with siblings
* <Comp x={...} /> | ||
* ``` | ||
*/ | ||
function isJSXAttribute( |
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.
Open to feedback on the name here.
*/ | ||
function isJSXAttribute( | ||
node: TSESTree.Node, | ||
): node is TSESTree.JSXExpressionContainer | TSESTree.JSXSpreadAttribute { |
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 like this (and the others) could just be boolean (especially the ones that are asserting more than just a type).
node: TSESTree.Node, | ||
): node is TSESTree.JSXExpressionContainer | TSESTree.JSXSpreadAttribute { | ||
return ( | ||
node.type === AST_NODE_TYPES.JSXExpressionContainer || |
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.
This is technically the RHS of a JSXAttribute
, so this check could also recurse one level higher if that is preferred. This should be the only way to have a function supplied in JSX though.
); | ||
} | ||
|
||
function isTypedParent( |
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.
Created a wrapping function for these to make it deduplicate between the common case and recursive object-property case. Open to feedback on the name here.
isPropertyOfObjectWithType(parent) || | ||
isFunctionArgument(parent, node) || | ||
isConstructorArgument(parent) |
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.
DRYing these out, it makes it more obvious that the recursive object-property doesn't include this base case, but it seems like it should?
Happy to move it into isTypedParent()
if this is indeed a bug.
{ | ||
code: ` | ||
const Comp: FC = () => { | ||
return <button {...{ onClick: () => {} }} />; |
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.
This is a cursed pattern but included for completeness.
TEMPORARY FORK PR for demo and notes
Addresses typescript-eslint#7552
https://typescript-eslint.io/contributing/pull-requests
PR Checklist
allowTypedFunctionExpressions
option should be aware of JSX typescript-eslint/typescript-eslint#7552Overview
allowTypedFunctionExpressions
option of theexplicit-function-return-type
rule