Introduce lint rule: no-invalid-bodytext-parent#2997
Introduce lint rule: no-invalid-bodytext-parent#2997marcysutton wants to merge 6 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 2fcab4a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +1.31 kB (+1.08%) Total Size: 123 kB 📦 View Changed
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-dhbduqtijn.chromatic.com/ Chromatic results:
|
marcysutton
force-pushed
the
WB-2247-bodytext-lint
branch
from
90f9f4b to
4570311
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
marcysutton
changed the title
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
marcysutton
force-pushed
the
WB-2247-bodytext-lint
branch
2 times, most recently
from
f7321bf to
5b4aed3
Compare
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (d1d8e0b) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2997"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2997 |
|
@claude review once |
| // practices | ||
| "plugin:storybook/recommended", | ||
| ], | ||
| plugins: ["import", "jest", "jsdoc", "promise", "monorepo", "react-hooks"], | ||
| plugins: [ | ||
| "import", | ||
| "jest", | ||
| "jsdoc", | ||
| "promise", | ||
| "monorepo", | ||
| "react-hooks", | ||
| "@khanacademy/wonder-blocks", | ||
| ], |
There was a problem hiding this comment.
🔴 The @khanacademy/wonder-blocks plugin is added to .eslintrc.js's plugins array but no rule from it (e.g., "@khanacademy/wonder-blocks/no-invalid-bodytext-parent": "error") is added to the rules section. As a result, the new lint rule will never fire when running pnpm lint on the monorepo — the primary stated feature of this PR is non-functional.
Extended reasoning...
The bug is straightforward: ESLint plugins do not auto-enable their rules upon registration. Registering a plugin in the plugins array only makes the plugin's rules available — each rule must be independently activated in the rules section with an explicit severity level ("warn", "error", etc.).
In .eslintrc.js, the diff adds "@khanacademy/wonder-blocks" to the plugins array (line 19), but no corresponding rule entry like "@khanacademy/wonder-blocks/no-invalid-bodytext-parent": "error" is added anywhere in the rules block. A search of the entire .eslintrc.js for any wonder-blocks or no-invalid-bodytext-parent entry in the rules section returns nothing.
The plugin source itself does not provide automatic activation: recommended.ts has rules: {} (empty), and strict.ts enables no-custom-tab-role but not no-invalid-bodytext-parent. The @khanacademy/eslint-config base config also does not enable this rule.
The impact is that the entire lint enforcement goal of this PR — preventing BodyText from being nested inside invalid HTML parents — is silently dead on arrival. Developers will not receive lint errors or auto-fix suggestions when they write invalid nesting patterns in the WB codebase. The PR description acknowledges this with "Run the lint rule in WB and frontend (In progress)", confirming the activation step was deferred but not completed.
Step-by-step proof: (1) Plugin is registered: plugins: [..., "@khanacademy/wonder-blocks"] in .eslintrc.js. (2) ESLint resolves the plugin and loads the rule implementations. (3) ESLint checks the rules section for @khanacademy/wonder-blocks/no-invalid-bodytext-parent — finds no entry. (4) Rule severity defaults to "off". (5) Running pnpm lint on any file containing <BodyText> inside a <button> or <p> produces zero lint errors.
The fix is a one-line addition to the rules block: "@khanacademy/wonder-blocks/no-invalid-bodytext-parent": "error". This should be added before merging so the rule is actually enforced in the codebase.
| function buildSpanTagFix( | ||
| fixer: TSESLint.RuleFixer, | ||
| openingElement: TSESTree.JSXOpeningElement, | ||
| ): TSESLint.RuleFix { | ||
| const existingTagAttr = openingElement.attributes.find( | ||
| (a): a is TSESTree.JSXAttribute => | ||
| a.type === "JSXAttribute" && | ||
| a.name.type === "JSXIdentifier" && | ||
| (a.name as TSESTree.JSXIdentifier).name === "tag", | ||
| ); | ||
|
|
||
| if (existingTagAttr?.value) { | ||
| return fixer.replaceText(existingTagAttr.value, '"span"'); | ||
| } | ||
|
|
||
| const lastAttr = | ||
| openingElement.attributes[openingElement.attributes.length - 1]; | ||
| return lastAttr | ||
| ? fixer.insertTextAfter(lastAttr, ' tag="span"') | ||
| : fixer.insertTextAfter(openingElement.name, ' tag="span"'); | ||
| } |
There was a problem hiding this comment.
🔴 The buildSpanTagFix autofix function has two correctness bugs. First, when BodyText has a boolean tag attribute (<BodyText tag />), the null-value check existingTagAttr?.value fails and the code appends tag="span" after the existing boolean attribute, producing the invalid duplicate-prop JSX <BodyText tag tag="span" />. Second and more critically, when BodyText has a dynamic expression like tag={isLabel ? 'span' : 'code'}, the check succeeds (JSXExpressionContainer is truthy) and fixer.replaceText(existingTagAttr.value, '"span"') silently destroys the developer's conditional logic, replacing it with a static string—running eslint --fix would corrupt code semantics without warning.
Extended reasoning...
Bug 1 — Boolean attribute produces duplicate tag props
In buildSpanTagFix (line 176), the guard is if (existingTagAttr?.value). For a boolean JSX attribute like <BodyText tag />, the AST represents the attribute with value: null. So existingTagAttr is found (non-null), but existingTagAttr.value is null, making the check falsy. The function falls through to the insertTextAfter(lastAttr, ' tag="span"') branch—but at this point lastAttr IS the boolean tag attribute. The fix inserts tag="span" immediately after it, producing <BodyText tag tag="span" />, which is invalid JSX with duplicate props.
Step-by-step proof: Input: <label><BodyText tag>text</BodyText></label>. (1) hasInlineTag returns false because getAttributeStringValue returns null for a boolean attribute. (2) Rule fires, calls buildSpanTagFix. (3) existingTagAttr = the boolean tag JSXAttribute with value === null. (4) existingTagAttr?.value → null → falsy, branch not taken. (5) lastAttr = that same boolean tag attribute. (6) Result: <BodyText tag tag="span">text</BodyText> — duplicate tag props.
Addressing the refutation: Reviewers correctly note that TypeScript would reject <BodyText tag /> since tag?: string does not accept a boolean. This reduces real-world impact because TypeScript type-checking would catch this pattern first. However, ESLint runs independently of the TypeScript compiler and processes syntactically valid JSX regardless of type correctness. The fixer logic is still incorrect, and if it runs (e.g., in a loosely-typed file, or before TS errors are fixed), it produces broken output. This warrants nit severity at minimum.
Bug 2 — Autofix destructively overwrites dynamic tag expressions
When BodyText has a non-literal tag expression like tag={isLabel ? 'span' : 'code'}, getAttributeStringValue returns null (because the JSXExpressionContainer's expression is a ConditionalExpression, not a Literal), so hasInlineTag returns false and the rule fires. In buildSpanTagFix, existingTagAttr.value is the JSXExpressionContainer object, which is truthy. The branch fixer.replaceText(existingTagAttr.value, '"span"') then replaces the entire {isLabel ? 'span' : 'code'} with the static string "span".
Step-by-step proof: Input: <button><BodyText tag={isLabel ? 'span' : 'code'}>text</BodyText></button>. (1) getAttributeStringValue returns null — isLabel ? 'span' : 'code' is not a Literal. (2) hasInlineTag returns false. (3) Rule fires. (4) buildSpanTagFix finds existingTagAttr.value = JSXExpressionContainer (truthy). (5) fixer.replaceText(existingTagAttr.value, '"span"') executes. (6) Output: <button><BodyText tag="span">text</BodyText></button> — the conditional logic is gone.
Why existing code doesn't prevent it: The if (existingTagAttr?.value) check was intended to distinguish "has a value to replace" from "no value, need to insert," but it does not distinguish between literal values (safe to replace) and dynamic expressions (unsafe to clobber). Any truthy JSX attribute value—whether a string literal, a simple variable reference, or a complex conditional—takes the replacement path.
Fix: For bug 1, check existingTagAttr (not its value) and call fixer.replaceText(existingTagAttr, 'tag="span"') to replace the whole attribute node. For bug 2, before replacing, check whether existingTagAttr.value is a Literal or a JSXExpressionContainer wrapping a Literal; if it's any other expression type, omit the fix callback entirely so eslint --fix leaves the code untouched and only the lint warning is shown.
… for no-invalid-bodytext-parent to avoid nested paragraphs
marcysutton
force-pushed
the
WB-2247-bodytext-lint
branch
from
5b4aed3 to
906f957
Compare
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
Great, thanks for working on our next custom lint rule! I left some questions and notes on how we can document the lint rule 😄 Let me know if you have any questions!
On a side note, it would be neat if we had a tool that could set up the templating for a new lint rule (the files for docs, tests, demo example, etc!). Maybe I'll explore how we could do that next! It would be nice for setting up new packages and components too!
| "@types/react-window": "^1.8.8", | ||
| "@typescript-eslint/eslint-plugin": "8.17.0", | ||
| "@typescript-eslint/parser": "8.17.0", | ||
| "@typescript-eslint/rule-tester": "^8.46.3", |
There was a problem hiding this comment.
This is set in the package.json for the eslint-plugin-wonder-blocks package, so I think it isn't needed at the root level!
|
|
||
| const rules = { | ||
| "no-custom-tab-role": noCustomTabRole, | ||
| "no-invalid-bodytext-parent": noInvalidBodyTextParent, |
There was a problem hiding this comment.
Do you think this rule should be added to the recommended or strict config?
There was a problem hiding this comment.
I'm leaning toward strict since the rollout in classic apps could be rough. But I could run a test next week to decide?
| invalid: [ | ||
| // --- WB Form: Choice --- | ||
| { | ||
| code: `<Choice label={<BodyText>Title</BodyText>} />`, |
There was a problem hiding this comment.
Do we want to handle used with LabeledField props too (label, descripton, errorMessage, contextLabel, readOnlyMessage, additionalHelperMessage)?
There was a problem hiding this comment.
Same with props for RadioGroup and CheckboxGroup (description and errorMessage props)!
|
|
||
| // --- HTML heading elements --- | ||
| { | ||
| code: `<h1><BodyText>Sub text</BodyText></h1>`, |
There was a problem hiding this comment.
Do we want test cases for StyledH1 elements?
| @@ -0,0 +1,343 @@ | |||
| /** | |||
There was a problem hiding this comment.
Lots of test coverage, nice!!
| "Disallow BodyText with a block-level tag (default <p>) inside elements that cannot contain block-level content", | ||
| recommended: true, | ||
| }, | ||
| fixable: "code", |
There was a problem hiding this comment.
Cool! Our first fixable custom wb lint rule!
| * Inline tags that make BodyText safe to nest inside inline/interactive | ||
| * contexts (e.g., inside a button, label, or paragraph element). | ||
| */ | ||
| const INLINE_BODY_TEXT_TAGS = new Set([ |
There was a problem hiding this comment.
I like how you've made it easy to configure different elements/components!
| output: `<Choice label={<BodyText tag="span">Title</BodyText>} />`, | ||
| }, | ||
| { | ||
| code: `<Choice description={<BodyText>Description</BodyText>} />`, |
There was a problem hiding this comment.
I see we don't need to define what props BodyText might be used with (label or description props)! Does that mean it'll check all props for where BodyText without an inline tag shouldn't be used ?
|
|
||
| const createRule = ESLintUtils.RuleCreator<WonderBlocksPluginDocs>( | ||
| (name) => | ||
| `https://github.com/Khan/wonder-blocks/blob/main/packages/eslint-plugin-wonder-blocks/docs/${name}.md`, |
There was a problem hiding this comment.
Could you add some docs for the rule in eslint-plugin-wonder-blocks/docs/? Lint errors will include this link with the error message!
Here is an example for a rule doc!
Then we can reference that doc so it shows up in the WB docs. Here is an example of how to do that: https://github.com/Khan/wonder-blocks/blob/main/__docs__/tools/eslint-plugin-wonder-blocks/no-custom-tab-role.mdx
There was a problem hiding this comment.
Please also add the rule to the table in the package README!
wonder-blocks/packages/eslint-plugin-wonder-blocks/README.md
Lines 67 to 73 in 1b5b57f
(the package README is already set up to be used in the WB SB docs 😄)
There was a problem hiding this comment.
yes, good call! I added those!
| output: null, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
Could you add some of these as examples in the demo project? It will help us verify that the lint rules are being integrated and triggered properly!
3 participants
Summary:
Adds new lint rule for the BodyText Typography component to disallow certain parents:
WB Components
Choice,Checkbox,Radio— render a<label>internally;<p>can't be inside a<label>Button,ActivityButton— inline/button context;<p>can't be inside<button>Heading,HeadingLarge,HeadingMedium,HeadingSmall,HeadingXSmall— heading elements can't contain block-level children (question: should we include the older components?)HTML Elements
<button>— inline/interactive element<p>— block element, can't contain another block element<label>— inline element<h1>–<h6>— heading elementsStyled/custom variants
StyledButtonStyledPStyledLabelBodyText itself
<BodyText>nested inside another<BodyText>— both default to<p>, which would be<p><p>(invalid HTML). Exception: if the outer BodyText has a block-containertagprop (div,section,article, etc.), it's fine.Questions:
no-invalid-bodytext-parent?Issue: WB-2247
Test plan: