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

fix(eslint-plugin): support JSX attrs in allowTypedFunctionExpressions #2

Closed
wants to merge 1 commit into from

Conversation

merrywhether
Copy link
Owner

@merrywhether merrywhether commented Aug 28, 2023

TEMPORARY FORK PR for demo and notes

Addresses typescript-eslint#7552

https://typescript-eslint.io/contributing/pull-requests

PR Checklist

Overview

  • Adds support for JSX attributes to the allowTypedFunctionExpressions option of the explicit-function-return-type rule
  • DRYes out code between common and recursive object-property cases

* foo(() => 1)
* ```
*/
function isFunctionArgument(
Copy link
Owner Author

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(
Copy link
Owner Author

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 {
Copy link
Owner Author

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 ||
Copy link
Owner Author

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(
Copy link
Owner Author

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)
Copy link
Owner Author

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: () => {} }} />;
Copy link
Owner Author

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.

@merrywhether merrywhether changed the title Support JSX attributes in allowTypedFunctionExpressions fix(eslint-plugin): support JSX attrs in allowTypedFunctionExpressions Aug 28, 2023
@merrywhether
Copy link
Owner Author

merrywhether commented Aug 28, 2023

typescript-eslint#7553

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
1 participant