-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: master
Are you sure you want to change the base?
Changes from 8 commits
40cdaaa
a4fb0a4
da04e17
23c5e34
27d8075
9c03d56
34d2e33
82b4da7
6655950
b420cb5
cf6a63f
cb7e496
e852af0
82ea0aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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} */ | ||
|
@@ -38,6 +39,10 @@ module.exports = { | |
default: false, | ||
type: 'boolean', | ||
}, | ||
ignorePrivate: { | ||
default: false, | ||
type: 'boolean', | ||
}, | ||
}, | ||
additionalProperties: false, | ||
}], | ||
|
@@ -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']); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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; | ||
} | ||
if (node.declaration.declarations) { | ||
const declarator = node.declaration.declarations.find((declarator) => validIdentifiers.has(declarator.init.type)); | ||
if (declarator) { | ||
return declarator.id.name; | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* 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 | ||
|
@@ -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) => !isIgnored(component) && !isPrivate(component)) | ||
.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)); | ||
}; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Switched over. |
||
|
||
return rule; | ||
}), | ||
}; |
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 as
exportOnly
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.