-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Bug fix for false positives with no-multi-comp #1089
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
Conversation
There was a problem hiding this 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.
tests/lib/rules/no-multi-comp.js
Outdated
' return createElement("img");', | ||
'};' | ||
].join('\n'), | ||
parser: 'babel-eslint' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'
.
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.
14a212d
to
c038899
Compare
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. |
If you'd like to add your test in a PR, that'd be great :-) |
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.