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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/eslint-plugin/docs/rules/explicit-function-return-type.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,10 @@ functionWithObjectArg({
return 1;
},
});

const Comp: FC = () => {
return <button onClick={() => {}} />;
};
```

### `allowHigherOrderFunctions`
Expand Down
75 changes: 47 additions & 28 deletions packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,51 @@ function isPropertyDefinitionWithTypeAnnotation(
);
}

/**
* Checks if a node belongs to:
* ```
* 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

parent: TSESTree.Node,
callee?: FunctionExpression,
): parent is TSESTree.CallExpression {
return (
parent.type === AST_NODE_TYPES.CallExpression &&
// make sure this isn't an IIFE
parent.callee !== callee
);
}

/**
* Checks if a node is an expression in a JSX attribute assignment
* ```
* <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.

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).

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.

node.type === AST_NODE_TYPES.JSXSpreadAttribute
);
}

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.

parent: TSESTree.Node,
callee?: FunctionExpression,
): boolean {
return (
isTypeAssertion(parent) ||
isVariableDeclaratorWithTypeAnnotation(parent) ||
isPropertyDefinitionWithTypeAnnotation(parent) ||
isFunctionArgument(parent, callee) ||
isJSXAttribute(parent)
);
}

/**
* Checks if a node belongs to:
* ```
Expand Down Expand Up @@ -78,13 +123,7 @@ function isPropertyOfObjectWithType(
return false;
}

return (
isTypeAssertion(parent) ||
isPropertyDefinitionWithTypeAnnotation(parent) ||
isVariableDeclaratorWithTypeAnnotation(parent) ||
isFunctionArgument(parent) ||
isPropertyOfObjectWithType(parent)
);
return isTypedParent(parent) || isPropertyOfObjectWithType(parent);
}

/**
Expand Down Expand Up @@ -127,23 +166,6 @@ function doesImmediatelyReturnFunctionExpression({
);
}

/**
* Checks if a node belongs to:
* ```
* foo(() => 1)
* ```
*/
function isFunctionArgument(
parent: TSESTree.Node,
callee?: FunctionExpression,
): parent is TSESTree.CallExpression {
return (
parent.type === AST_NODE_TYPES.CallExpression &&
// make sure this isn't an IIFE
parent.callee !== callee
);
}

/**
* Checks if a function belongs to:
* ```
Expand Down Expand Up @@ -194,11 +216,8 @@ function isTypedFunctionExpression(
}

return (
isTypeAssertion(parent) ||
isVariableDeclaratorWithTypeAnnotation(parent) ||
isPropertyDefinitionWithTypeAnnotation(parent) ||
isTypedParent(parent, node) ||
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.

);
}
Expand Down
62 changes: 62 additions & 0 deletions packages/eslint-plugin/tests/rules/explicit-function-return-type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,46 @@ class App {
`,
options: [{ allowTypedFunctionExpressions: true }],
},
// https://github.com/typescript-eslint/typescript-eslint/issues/7552
{
code: `
const Comp: FC = () => {
return <button onClick={() => {}} />;
};
`,
options: [{ allowTypedFunctionExpressions: true }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
},
{
code: `
const Comp: FC = () => {
return <Button on={{ click: () => {} }} />;
};
`,
options: [{ allowTypedFunctionExpressions: true }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
},
{
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.

};
`,
options: [{ allowTypedFunctionExpressions: true }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
},
// https://github.com/typescript-eslint/typescript-eslint/issues/525
{
code: `
Expand Down Expand Up @@ -977,6 +1017,28 @@ const x: Foo = {
},
],
},
{
code: `
const Comp = (): JSX.Element => {
return <button onClick={() => {}} />;
};
`,
options: [{ allowTypedFunctionExpressions: false }],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
errors: [
{
messageId: 'missingReturnType',
line: 3,
endLine: 3,
column: 30,
endColumn: 32,
},
],
},
{
code: '() => () => {};',
options: [{ allowHigherOrderFunctions: true }],
Expand Down