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] `no-unused-prop-types`: false positive with callback #2375

Merged
merged 1 commit into from Sep 6, 2019

Conversation

@golopot
Copy link
Contributor

commented Aug 6, 2019

Fixes #2350

This pr fixes getParentStatelessComponent when the current traversed node is like a callback (is a function and is an argument). The function getParentStatelessComponent is revised to use a more whitelist-ish approach.

Fixes false positive like:

function Foo(props) {
  useEffect(() => {
    const { a } = props;
    document.title = a;
  });
  
  return <p/>;
}
  Foo.propTypes = {
  a: PropTypes.string,
}
tests/lib/rules/no-this-in-sfc.js Show resolved Hide resolved
case 'AssignmentExpression':
case 'Property':
case 'ReturnStatement':
case 'ExportDefaultDeclaration': {

This comment has been minimized.

Copy link
@golopot

golopot Aug 6, 2019

Author Contributor

These cases are

const Foo = () => <p />;

module.exports = function Foo() {};

module.exports = {
  Foo() {},
};

function hof() {
  return function Foo() {};
}

export default function Foo() {}

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 19, 2019

Collaborator

Can we add explicit test cases for each of them?

This comment has been minimized.

Copy link
@golopot

golopot Aug 19, 2019

Author Contributor

Currently the tests cases for them are scattered in several rules, do you want a set of test cases centralized in, say, tests/rules/prop-types.js?

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 19, 2019

Collaborator

it'd probably be ideal for every rule that uses component detection to have all of these cases, tbh

@golopot

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

The tests are broken by the merge of #2184, which added a test case:

}, {
code: `
const HelloComponent = (0, (props) => {
return <div></div>;
});

Do we want to support components defined in comma operator? (IMO, no.)

@ljharb ljharb force-pushed the golopot:issue-2350 branch from aac7807 to cfa6135 Aug 31, 2019

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 31, 2019

Yes, we should support components in any expression position.

@golopot golopot force-pushed the golopot:issue-2350 branch 3 times, most recently from 73bb36d to 47e8640 Sep 3, 2019

lib/util/Components.js Outdated Show resolved Hide resolved
lib/util/Components.js Outdated Show resolved Hide resolved
lib/util/Components.js Outdated Show resolved Hide resolved
lib/util/Components.js Outdated Show resolved Hide resolved
@ljharb
ljharb approved these changes Sep 6, 2019

@ljharb ljharb added the bug label Sep 6, 2019

@ljharb ljharb force-pushed the golopot:issue-2350 branch from bd90a9b to 66725bc Sep 6, 2019

@ljharb ljharb merged commit 66725bc into yannickcr:master Sep 6, 2019

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.04%) to 97.528%
Details

@golopot golopot deleted the golopot:issue-2350 branch Sep 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.