fix(eslint-plugin): [explicit-function-return-type] support JSX attributes in allowTypedFunctionExpressions
#7553
Conversation
|
Thanks for the PR, @merrywhether! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| * <Comp x={...} /> | ||
| * ``` | ||
| */ | ||
| function isJSXAttribute( |
There was a problem hiding this comment.
Open to feedback on the name here.
| node: TSESTree.Node, | ||
| ): node is TSESTree.JSXExpressionContainer | TSESTree.JSXSpreadAttribute { | ||
| return ( | ||
| node.type === AST_NODE_TYPES.JSXExpressionContainer || |
There was a problem hiding this comment.
This is technically the relevant RHS type of JSXAttribute, so this check could also recurse one level higher if that is preferred. This should be the only way to directly supply a function in JSX though.
There was a problem hiding this comment.
I think we'll want to check the parent because this is another valid location for a JSXExpressionContainer:
<>{() => {}}</>There was a problem hiding this comment.
@bradzacher Ah, I totally forgot about the render-props pattern, it's been a while since their heyday! Running this down actually led me across an existing issue in TS where fragment shorthands are not typed properly: microsoft/TypeScript#50429
I went around on this for a while, but I again think all JSXExpressionContainer instances are fully bound by types barring that issue (reference CodeSandbox with render props):
- Attribute assignment: original case; type is constrained by assignment
- Render props of built-ins and
<Fragment />: not allowed due to limited child types (JSX.ElementTypeorJSX.Element | null); all functions will be type errors here - Render props of
<>fragment shorthand: all functions should be type errors here - Render props of components with custom function children: properly typed-constrained by explicit
childrenprop, the same as if children was defined via attribute
I eventually realized that while something like this is unsafe:
<>{() => 'bug'}</>Having a lint error that turns it into this doesn't make things better as it's still not a type error but won't work as expected at runtime (especially if you return an object instead):
<>{(): string => 'bug'}</>While this is a currently an issue with TypeScript, it doesn't seem like the lint rule can do anything better here in place of proper type-checking. But functions can be valid in children expression containers generally.
| isPropertyOfObjectWithType(parent) || | ||
| isFunctionArgument(parent, node) || | ||
| isConstructorArgument(parent) |
There was a problem hiding this comment.
DRYing these out, it makes it more obvious that the recursive object-property case doesn't include this isConstructorArgument() case, but it seems like it should?
Happy to move it into isTypedParent() and add tests if this is indeed a bug.
bradzacher
changed the title
allowTypedFunctionExpressions
| node: TSESTree.Node, | ||
| ): node is TSESTree.JSXExpressionContainer | TSESTree.JSXSpreadAttribute { | ||
| return ( | ||
| node.type === AST_NODE_TYPES.JSXExpressionContainer || |
There was a problem hiding this comment.
I think we'll want to check the parent because this is another valid location for a JSXExpressionContainer:
<>{() => {}}</>| { | ||
| code: ` | ||
| const Comp: FC = () => { | ||
| return <button {...{ onClick: () => {} }} />; | ||
| }; | ||
| `, | ||
| options: [{ allowTypedFunctionExpressions: true }], | ||
| parserOptions: { | ||
| ecmaFeatures: { | ||
| jsx: true, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
for completeness can we also get this as an invalid test?
There was a problem hiding this comment.
Added negated counterparts of all new tests
bradzacher
added
the
awaiting response
merrywhether
force-pushed
the
explicit-return-type-jsx
branch
3 times, most recently
from
9b08f41
to
9abccf6
Compare
|
@bradzacher Updated this, LMK what you think |
github-actions
bot
removed
the
awaiting response
merrywhether
force-pushed
the
explicit-return-type-jsx
branch
from
7ef3ec4
to
52ff72c
Compare
|
@bradzacher Anything I can do to help land this? |
PR Checklist
allowTypedFunctionExpressionsoption should be aware of JSX #7552Overview
Adds support for JSX attributes to the
allowTypedFunctionExpressionsoption of theexplicit-function-return-typerule by augmenting theisTypedFunctionExpression()function to include more parent types. Also DRYes out code between common and recursive object-property cases.