[New] create ignorePrivate option for no-multi-comp rule #3842
Conversation
vwillyams
changed the title
docs/rules/no-multi-comp.md
Outdated
| @@ -69,6 +69,55 @@ class HelloJohn extends React.Component { | |||
| module.exports = HelloJohn; | |||
| ``` | |||
|
|
|||
| ### `ignorePrivate` | |||
There was a problem hiding this comment.
"private" might not be the best name here. maybe "ignoreNonExported"?
There was a problem hiding this comment.
Yeah I originally had this labeled as exportOnly but wanted to stick closer to the ignoreStateless convention already established.
There was a problem hiding this comment.
ignoreNonExported feels a little clunky to me but I can't think of anything better. just one of those hard problems in software.
There was a problem hiding this comment.
yeah totally :-) "ignoreInternal"?
There was a problem hiding this comment.
Sounds good to me, switched over.
| const ignorePrivate = configuration.ignorePrivate || false; | ||
|
|
||
| const exportedComponents = new Set(); // Track exported components | ||
| const validIdentifiers = new Set(['ArrowFunctionExpression', 'Identifier', 'FunctionExpression']); |
There was a problem hiding this comment.
i believe in react 19, async functions can be components as well.
There was a problem hiding this comment.
Yeah good point, I had not thought about RSCs.
There was a problem hiding this comment.
Added a little coverage for this - I'm not an RSC dev by day though so may have made some oversights.
lib/rules/no-multi-comp.js
Outdated
| if (ignorePrivate) { | ||
| rule.ExportNamedDeclaration = (node) => { | ||
| exportedComponents.add(getExportedComponentName(node)); | ||
| }; | ||
|
|
||
| rule.ExportDefaultDeclaration = (node) => { | ||
| exportedComponents.add(getExportedComponentName(node)); | ||
| }; | ||
| } |
There was a problem hiding this comment.
instead of mutating maybe we could use Object.assign here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
it probably doesn't matter much at all, but mutation generally risks slowdowns later, as opposed to creating objects all at once.
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
|
FYI I will hopefully find time to have this ready for merge in the next 2-3 days. I'm going to get this working with RSCs and I also want to look at making the tests programmatically generated so that we can iterate over every possible permutation without having to write them all out individually (and risk missing something). In practice there's likely to be diminishing returns on that kind of testing but I assume that individual test cases are very cheap here and I don't really like the maintenance burden of the test cases I've added in this PR. |
|
FYI I'm currently iterating on the test coverage and making sure this works in all cases. I didn't entirely expect the amount of complexity that would go into this feature! But it's been a fun learning process. |
Adds a new option to no-multi-comp which ignores components that are not exported.
Currently WIP:
npm testwasn't working locally so I'm hoping to rely on CI for this.