feat(eslint): Add restrict-jsx-slot-children lint rule#109169
feat(eslint): Add restrict-jsx-slot-children lint rule#109169
Conversation
…ect slots Adds a configurable ESLint rule that restricts JSX trees passed to specific slot props to a declared set of allowed elements. Each allowed element is declared with a source package, an import kind (member expression like MenuComponents.* or named identifier like Flex), and a role (leaf to stop recursion, wrapper to recurse into children). The rule walks the entire JSX tree — not just the root — so <Flex><Button/></Flex> is caught even though Flex itself is allowed. Top-level ternaries and logical expressions are also unwrapped. Activated for CompactSelect's menuHeaderTrailingItems and menuFooter: only MenuComponents.* from @sentry/scraps/compactSelect and layout primitives (Flex, Stack, Grid, Container) from @sentry/scraps/layout are permitted. New slot configurations can be added in eslint.config.mjs without touching the rule implementation. Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
bot
added
the
Scope: Frontend
…om exports
Collapse the two-variant allowed-element schema (member/named with
explicit type and role fields) into a single unified shape:
{ source, names }
where names is a flat list of allowed JSX tag names using dot notation
for member expressions (e.g. 'MenuComponents.ApplyButton') and plain
strings for identifier elements (e.g. 'Flex').
Leaf vs wrapper behaviour is now implicit: member expressions stop
recursion, identifiers recurse into children. Import aliasing is still
tracked via the source field.
Also rename propName → propNames (array) so one config block can cover
multiple props, and remove the rule from the scraps plugin exports and
eslint.config.mjs until it is ready to land.
Co-Authored-By: Claude <noreply@anthropic.com>
… rule Without a component name filter the rule would flag any JSX element whose prop name matched the configured propNames (e.g. menuFooter), even on unrelated components. Add an optional componentNames field to each slot config entry so the rule only checks slots on the specified components (e.g. CompactSelect). Co-Authored-By: Claude <noreply@anthropic.com>
… config Register the restrict-jsx-slot-children rule in the scraps plugin and add a pre-configured compactSelect config that enforces allowed slot children for CompactSelect's menuHeaderTrailingItems and menuFooter props. Spread the config into eslint.config.mjs via the plugin's configs export. Co-Authored-By: Claude <noreply@anthropic.com>
JonasBa
force-pushed
the
jb/eslint/restrict-jsx-slot-children
branch
from
e3b13be to
adad13d
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
static/eslint/eslintPluginScraps/src/rules/restrict-jsx-slot-children.mjs
Show resolved
Hide resolved
static/eslint/eslintPluginScraps/src/rules/restrict-jsx-slot-children.mjs
Show resolved
Hide resolved
Split the menuHeaderTrailingItems slot from menuFooter: the shared slot no longer allows ClearButton and ResetButton, while a new dedicated slot for menuHeaderTrailingItems permits them along with layout components (Flex, Stack, Grid, Container). Co-Authored-By: Claude <noreply@anthropic.com>
static/eslint/eslintPluginScraps/src/rules/restrict-jsx-slot-children.mjs
Show resolved
Hide resolved
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| // Pre-computed hint string produced by buildAllowedHint() for the config above | ||
| const ALLOWED_HINT = | ||
| "MenuComponents.HeaderButton, MenuComponents.LinkButton, MenuComponents.CTAButton, MenuComponents.CTALinkButton, MenuComponents.ApplyButton, MenuComponents.CancelButton, MenuComponents.Alert from '@sentry/scraps/compactSelect', or Flex, Stack, Grid, Container from '@sentry/scraps/layout'"; |
There was a problem hiding this comment.
can we read the allowed things from the conf ? This feels like it can get out of sync quickly otherwise
static/eslint/eslintPluginScraps/src/rules/restrict-jsx-slot-children.spec.mjs
Show resolved
Hide resolved
| propNames: ['menuHeaderTrailingItems', 'menuFooter'], | ||
| allowed: [ | ||
| { | ||
| source: '@sentry/scraps/compactSelect', |
There was a problem hiding this comment.
[Question] Have you considered a way for components to self-register? Say, custom JSDoc comments on the components themselves?
Although JSDoc parsing is disabled on the typescript-eslint parser side, TypeScript itself still has access to & IIRC some level of parsing for JSDoc comments.
There was a problem hiding this comment.
@JoshuaKGoldberg that could be very interesting. I've intentionally kept this this simple and dumb so that I could first validate how well this approach would even work in practice (cross referencing by manually looking at the 20 something instances in the product).
I'll do some research and give this a stab in a followup PR, I think it'd be a great improvement! Are you perhaps aware of some prior art that uses this approach?
There was a problem hiding this comment.
For self-registration specifically, I can't remember any prior art off the top of my head 🤔. But I'll try to back-of-mind think on it.
For generally specifying things, maybe https://typescript-eslint.io/packages/type-utils/type-or-value-specifier would be useful? I think it's a little more general-purpose (and therefore verbose) than what you've got, but is kinda a similar idea?
There was a problem hiding this comment.
I see, thanks for sharing. I'd probably still prefer if this was self documenting as opposed to config driven, so let me see if I can get JSDoc inference working
…full tree Remove the member-expression leaf-stop and the memberAllowed/namedAllowed split. The rule now uses a single allowedNames set (plain names and dotted member names resolved at import time) and recurses into every allowed element's children, so invalid descendants are never silently skipped. React fragments (<Fragment>, <React.Fragment>, <>) are treated as transparent wrappers and bypass the allowed check entirely. Move the compactSelect slot config out of the plugin's configs export and inline it directly in eslint.config.mjs, fixing a duplicate slot entry for menuHeaderTrailingItems that was introduced when the two slots were split. Co-Authored-By: Claude <noreply@anthropic.com>
Cover <>, <Fragment>, and <React.Fragment> in both the valid and invalid sections. Each form is verified with multiple children, nested inside layout wrappers, and nested inside other fragments to confirm the rule descends into all fragment variants. Co-Authored-By: Claude <noreply@anthropic.com>
static/eslint/eslintPluginScraps/src/rules/restrict-jsx-slot-children.mjs
Show resolved
Hide resolved
Replace the static string with a derivation from COMPACT_SELECT_OPTIONS using the same formula as the rule internals, so the hint stays in sync automatically when the allowed list changes. Co-Authored-By: Claude <noreply@anthropic.com>
static/eslint/eslintPluginScraps/src/rules/restrict-jsx-slot-children.mjs
Show resolved
Hide resolved
|
I ran eslint with TIMING=1 flag to verify that there is no major performance impact to the linter workflow. Whatever the cost of this rule is, it's less than 2% (min cost from the output) |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
static/eslint/eslintPluginScraps/src/rules/restrict-jsx-slot-children.mjs
Show resolved
Hide resolved
static/eslint/eslintPluginScraps/src/rules/restrict-jsx-slot-children.mjs
Show resolved
Hide resolved
…children
The LogicalExpression handler checked && and || but not ??. In the ESTree
AST, ?? is also a LogicalExpression, so it silently fell through without
checking either operand — allowing patterns like menuFooter={x ?? <Banned/>}
to pass without a lint error.
Fix by treating ?? identically to ||: check both sides, since either can
be the value that gets rendered. Add valid and invalid test cases to cover
the new operator.
Co-Authored-By: Claude <noreply@anthropic.com>
|
If nobody minds, I would like to look at the JSDoc inference in a followup change - this initial PR should be enough to validate that this works before we go too deep on the ergonomics of the rule. |
| errors: [forbidden('Fragment', 'menuFooter')], | ||
| }, | ||
|
|
||
| // ── Custom component directly in slot ───────────────────────────────────── |
There was a problem hiding this comment.
We can definitely come back to this later if we get complaints.
Adds a configurable ESLint rule to `eslintPluginScraps` that restricts the JSX trees passed to specific slot props to a declared set of allowed elements. The rule is written in a generic way so that it can easily be extended to other component types. #109198 should address the violations and allow us to merge these changes --------- Co-authored-by: Claude <noreply@anthropic.com>
4 participants
Adds a configurable ESLint rule to
eslintPluginScrapsthat restricts the JSX trees passed to specific slot props to a declared set of allowed elements. The rule is written in a generic way so that it can easily be extended to other component types.#109198 should address the violations and allow us to merge these changes