Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
[New] create ignorePrivate option for no-multi-comp rule#3842
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
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.
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"?
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.
Yeah I originally had this labeled asexportOnly but wanted to stick closer to theignoreStateless convention already established.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yeah totally :-) "ignoreInternal"?
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 to me, switched over.
Uh oh!
There was an error while loading.Please reload this page.
| 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.
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.
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.
Yeah good point, I had not thought about RSCs.
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.
Added a little coverage for this - I'm not an RSC dev by day though so may have made some oversights.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
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?
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.
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.
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.
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.
Switched over.
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
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. |
Uh oh!
There was an error while loading.Please reload this page.
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. |
Uh oh!
There was an error while loading.Please reload this page.
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.