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

Bug fix for false positives with no-multi-comp #1089

Merged
merged 1 commit into from Feb 24, 2017

Conversation

benstepp
Copy link
Contributor

Previously, when createElement was destructured from React, it would
cause any function defined in the file to be flagged as a React
Component. This change makes it such that the function must call
createElement to be considered a component.

Fixes #1088


Source of bug:
416deff#diff-bafb3355a409c4ba076a9e3d609ad65bR294

Note: Not sure if this is the best place for this test. I feel like component detection testing should have its own place where it happens. It seems like you have to run the whole test suite to see if Components are still being detected correctly. Thankfully the whole test suite runs fairly quickly.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending one comment.

' return createElement("img");',
'};'
].join('\n'),
parser: 'babel-eslint'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case doesn't need babel-eslint - let's use the default parser whenever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used babel-eslint over sourceType: 'module' because it was way more popular in the codebase for handling import statements. (git grep sourceType)

Do you think it would be better to apply the module sourceType to the entire test file, just the single test using object.assign, or using const createElement = require('react').createElement; in the actual test?

Maybe a chore issue to clean out all of babel-eslint and replace it with sourceType module. eslint's default parser could handle a vast majority of the tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sourceType is required for anything using the default parser, but it's unnecessary for anything using babel-eslint.

Ideally, every test case that uses babel-eslint would also have a corresponding test case using the default parser, and babel-eslint tests would only be for features that eslint core can't handle (like flow types, or proposals)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically I think it should be applied to this specific test, and if we want to make things consistent holistically, that can be a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I went ahead and squashed a change with sourceType: 'module'.

@ljharb ljharb added the bug label Feb 23, 2017
Previously, when createElement was destructured from React, it would
cause any function defined in the file to be flagged as a React
Component. This change makes it such that the function must call
createElement to be considered a component.

Fixes jsx-eslint#1088

---

Review: Use object.assign with sourceType: module in the added test over
babel-eslint.
@ljharb ljharb merged commit b646485 into jsx-eslint:master Feb 24, 2017
@arian
Copy link
Contributor

arian commented Mar 1, 2017

This fixes another false positive for the display-name rule:

diff --git a/tests/lib/rules/display-name.js b/tests/lib/rules/display-name.js
index 3f8db25..83a3772 100644
--- a/tests/lib/rules/display-name.js
+++ b/tests/lib/rules/display-name.js
@@ -349,6 +349,17 @@ ruleTester.run('display-name', rule, {
     parser: 'babel-eslint'
   }, {
     code: [
+      'import React, {createElement} from "react";',
+      'const SomeComponent = (props) => {',
+      '  const {foo, bar} = props;',
+      '  return someComponentFactory({',
+      '    onClick: () => foo(bar("x"))',
+      '  });',
+      '};'
+    ].join('\n'),
+    parser: 'babel-eslint'
+  }, {
+    code: [
       'import React, {Component} from "react";',
       'function someDecorator(ComposedComponent) {',
       '  return class MyDecorator extends Component {',

I wrote a test for it, but it seems this pull request already fixed it in master. For the tagged version 6.10.0 this test fails.

@ljharb
Copy link
Member

ljharb commented Mar 1, 2017

If you'd like to add your test in a PR, that'd be great :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants