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

Added handling in no-multi-comp for forwardRef and memo wrapping comp… #2184

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jenil94
Copy link
Contributor

jenil94 commented Mar 1, 2019

Added handling in no-multi-comp for forwardRef and memo wrapping components declared in the same file.

Fixes #2172

Changes

While checking for isPragmaComponentWrapper code will also check whether the wrapper is wrapping existing component or creating a new one.

If it's is wrapping existing component in extra JSX element then it will be considered as a separate component else it won't.

In the example below the second abc won't be considered as extra component

class StoreListItem extends React.PureComponent {
	// A bunch of stuff here
}
const abc = React.forwardRef((props, ref) => <StoreListItem />)

but in the second case abc will be considered as a separate component

class StoreListItem extends React.PureComponent {
	// A bunch of stuff here
}
const abc = React.forwardRef((props, ref) => <div><StoreListItem /></div>)

jenil94 added some commits Mar 1, 2019

@jenil94 jenil94 marked this pull request as ready for review Mar 3, 2019

@ljharb
Copy link
Collaborator

ljharb left a comment

Can you confirm this will fix component detection for any case where a component is passed as the argument to a function?

Show resolved Hide resolved lib/util/Components.js Outdated
if (node.openingElement && node.openingElement.name && node.openingElement.name.name) {
return node.openingElement.name.name;
}
return null;

This comment has been minimized.

@ljharb

ljharb Mar 7, 2019

Collaborator

when would we expect to reach here, if it's a JSXElement node? An explicit exception would catch parser bugs more quickly.

* Getting the first JSX element's name.
*/
getNameOfWrappedComponent(node) {
if (node.length && node[0].body) {

This comment has been minimized.

@ljharb

ljharb Mar 7, 2019

Collaborator
Suggested change
if (node.length && node[0].body) {
if (node.length < 1) {
return null;
}
const body = node[0].body;
let componentName;
if (body.type === 'JSXElement') {
componentName = this.getComponentNameFromJSXElement(body);

This comment has been minimized.

@ljharb

ljharb Mar 7, 2019

Collaborator
Suggested change
componentName = this.getComponentNameFromJSXElement(body);
return this.getComponentNameFromJSXElement(body);
let componentName;
if (body.type === 'JSXElement') {
componentName = this.getComponentNameFromJSXElement(body);
} else if (body.type === 'BlockStatement') {

This comment has been minimized.

@ljharb

ljharb Mar 7, 2019

Collaborator
Suggested change
} else if (body.type === 'BlockStatement') {
}
if (body.type === 'BlockStatement') {
getDetectedComponents() {
const list = components.list();
const componentList = [];
Object.keys(list).forEach(key => {

This comment has been minimized.

@ljharb
Object.keys(list).forEach(key => {
const val = list[key];
if (val.node.type === 'ClassDeclaration') {
componentList.push(val.node.id.name);

This comment has been minimized.

@ljharb

ljharb Mar 7, 2019

Collaborator

could this be a map and a .filter(Boolean), rather than mutating an array inside a forEach?

* creating a new one.
*/
isJustAWrapper(node) {
const childComponent = this.getNameOfWrappedComponent(node.arguments);

This comment has been minimized.

@ljharb

ljharb Mar 7, 2019

Collaborator

Can you add a test for a fully anonymous SFC - ie, something for which component.displayName || component.name will be undefined?

* It will check wheater memo/forwardRef is wrapping existing component or
* creating a new one.
*/
isJustAWrapper(node) {

This comment has been minimized.

@ljharb

ljharb Mar 7, 2019

Collaborator

Naming is hard :-) what do you think about calling this nodeWrapsComponent?

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