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

[New] create ignorePrivate option for no-multi-comp rule #3842

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft
49 changes: 49 additions & 0 deletions docs/rules/no-multi-comp.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,55 @@ class HelloJohn extends React.Component {
module.exports = HelloJohn;
```

### `ignorePrivate`
Copy link
Member

Choose a reason for hiding this comment

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

"private" might not be the best name here. maybe "ignoreNonExported"?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I originally had this labeled as exportOnly but wanted to stick closer to the ignoreStateless convention already established.

Copy link
Author

Choose a reason for hiding this comment

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

ignoreNonExported feels a little clunky to me but I can't think of anything better. just one of those hard problems in software.

Copy link
Member

Choose a reason for hiding this comment

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

yeah totally :-) "ignoreInternal"?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good to me, switched over.


When `true` the rule will ignore components which are not exported, which allows you to define components that are consumed only within the same file.
This ensures there is only one entry point for a React component without limiting the structural content of the file.

Examples of **correct** code for this rule:

```jsx
export function Hello(props) {
return <div>Hello {props.name}</div>;
}
function HelloAgain(props) {
return <div>Hello again {props.name}</div>;
}
```

```jsx
function Hello(props) {
return <div>Hello {props.name}</div>;
}
class HelloJohn extends React.Component {
render() {
return <Hello name="John" />;
}
}
module.exports = HelloJohn;
```

Examples of **incorrect** code for this rule:

```jsx
export function Hello(props) {
return <div>Hello {props.name}</div>;
}
export function HelloAgain(props) {
return <div>Hello again {props.name}</div>;
}
```

```jsx
function Hello(props) {
return <div>Hello {props.name}</div>;
}
function HelloAgain(props) {
return <div>Hello again {props.name}</div>;
}
module.exports = {Hello, HelloAgain}
vwillyams marked this conversation as resolved.
Show resolved Hide resolved
```

## When Not To Use It

If you prefer to declare multiple components per file you can disable this rule.
75 changes: 71 additions & 4 deletions lib/rules/no-multi-comp.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const report = require('../util/report');

const messages = {
onlyOneComponent: 'Declare only one React component per file',
onlyOneExportedComponent: 'Declare only one exported React component per file',
};

/** @type {import('eslint').Rule.RuleModule} */
Expand All @@ -38,6 +39,10 @@ module.exports = {
default: false,
type: 'boolean',
},
ignorePrivate: {
default: false,
type: 'boolean',
},
},
additionalProperties: false,
}],
Expand All @@ -46,6 +51,43 @@ module.exports = {
create: Components.detect((context, components, utils) => {
const configuration = context.options[0] || {};
const ignoreStateless = configuration.ignoreStateless || false;
const ignorePrivate = configuration.ignorePrivate || false;

const exportedComponents = new Set(); // Track exported components
const validIdentifiers = new Set(['ArrowFunctionExpression', 'Identifier', 'FunctionExpression']);
Copy link
Member

Choose a reason for hiding this comment

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

i believe in react 19, async functions can be components as well.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah good point, I had not thought about RSCs.

Copy link
Author

Choose a reason for hiding this comment

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

Added a little coverage for this - I'm not an RSC dev by day though so may have made some oversights.


/**
* Given an export declaration, find the export name.
* @param {Object} node
* @returns {string}
*/
function getExportedComponentName(node) {
if (node.declaration.type === 'ClassDeclaration') {
return node.declaration.id.name;
}
for (const declarator of node.declaration.declarations || []) {
const type = declarator.init.type;
if (validIdentifiers.has(type)) {
return declarator.id.name;
}
}
vwillyams marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Given a React component, find the exported name.
* @param {Object} component
* @returns {string}
*/
function findComponentIdentifierFromComponent(component) {
let name;
if (component.node.parent.id) {
name = component.node.parent.id.name;
}
if (!name) {
name = component.node.id.name;
}
return name;
}

/**
* Checks if the component is ignored
Expand All @@ -61,21 +103,46 @@ module.exports = {
);
}

return {
/**
* Checks if the component is exported, if exportOnly is set
* @param {Object} component The component being checked.
* @returns {boolean} True if the component is exported or exportOnly is false
*/
function isPrivate(component) {
return ignorePrivate && exportedComponents.has(findComponentIdentifierFromComponent(component));
}

const rule = {
'Program:exit'() {
if (components.length() <= 1) {
return;
}

values(components.list())
.filter((component) => !isIgnored(component))
ljharb marked this conversation as resolved.
Show resolved Hide resolved
.filter((component) => !isPrivate(component))
vwillyams marked this conversation as resolved.
Show resolved Hide resolved
.slice(1)
.forEach((component) => {
report(context, messages.onlyOneComponent, 'onlyOneComponent', {
node: component.node,
});
report(context,
ignorePrivate ? messages.onlyOneExportedComponent : messages.onlyOneComponent,
ignorePrivate ? 'onlyOneExportedComponent' : 'onlyOneComponent',
{
node: component.node,
});
});
},
};

if (ignorePrivate) {
rule.ExportNamedDeclaration = (node) => {
exportedComponents.add(getExportedComponentName(node));
};

rule.ExportDefaultDeclaration = (node) => {
exportedComponents.add(getExportedComponentName(node));
};
}
Copy link
Member

Choose a reason for hiding this comment

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

instead of mutating maybe we could use Object.assign here?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah sure, sounds good. I'm not clear on why but I'm not super experienced in the eslint lifecycle and I can imagine that mutation may introduce issues somehow.

Copy link
Member

Choose a reason for hiding this comment

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

it probably doesn't matter much at all, but mutation generally risks slowdowns later, as opposed to creating objects all at once.

Copy link
Author

Choose a reason for hiding this comment

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

Switched over.


return rule;
}),
};
Loading
Loading