Repository navigation
lint(no-nullish-jsx-guard): catch isRenderable written out by hand, and enforce with a baseline - #9147
Conversation
…nd enforce with a baseline
The rule now also flags a chain that tests one value for nullishness or
the empty string AND for a boolean (Badge's
label !== '' && label != null && typeof label !== 'boolean', and the
negated == null || typeof === 'boolean' form) anywhere, not only as a JSX
guard: the boolean test marks the value as a rendered node, so the chain
is isRenderable by hand.
The repository config now enforces the rule in core and richtext at the
strict severity with {baseline, root}. The 38 guarded values in 27 files
that predate enforcement (JSX guards and hand-written checks) are listed
in no-nullish-jsx-guard.baseline.json, written by
no-nullish-jsx-guard.baseline.mjs (--check verifies), so main stays green
and a new one fails strict lint. isRenderable.ts itself is exempt.
Tests: Badge's, DropdownMenuGroup's and RichTextEditor's forms, the
negated form, baseline scoping by file and value, and what stays valid.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the new hand-written isRenderable check also reports data and union type switches, so it needs narrowing before it becomes a strict-lint error.
- The scalar switches
value == null || typeof value === 'boolean' || typeof value === 'string'inprotocol.ts,semanticFence.tsand the Tooltip browser spec, and themobileNavconfig-object test inAppShell.tsx, are not slot checks; skipping a chain that also tests the value'stypeofagainst another type (or callsisValidElementon it) clears them, after which the baseline can be regenerated. - The baseline is keyed by value per file, so a new guard of an already-listed value (for example
descriptioninItem.tsx) still passes; adjust "a new one is an error" in the description,eslint.config.jsand the README, or key the baseline by count. no-nullish-jsx-guard.baseline.mjsrepeatsno-internal-barrel-import.baseline.mjsalmost line for line, so one shared runner would keep the two baselines in step.
[Automated review]
| const typed = | ||
| parseBooleanTypeof(operand, equal) ?? | ||
| parseLiteralComparison(operand, equal, new Set([true, false])); | ||
| if (typed) { | ||
| booleans.set(sourceCode.getText(typed), typed); | ||
| } |
There was a problem hiding this comment.
Operands that test the same value against another type (typeof value === 'string', isValidElement(mobileNav)) are ignored, so type switches match too: protocol.ts:672, semanticFence.ts:99, the Tooltip browser spec :932 and the config-object test at AppShell.tsx:547 are all reported as isRenderable by hand. Following the message would change what those functions return. Consider skipping a chain that also tests the value with typeof against anything but 'boolean', or with isValidElement, and adding those shapes as valid tests.
| // block are listed in the baseline and stay allowed until migrated; a new | ||
| // one is reported (an error in strict mode, so a pull request that adds one | ||
| // fails lint, while main stays green). |
There was a problem hiding this comment.
The baseline is keyed by value text per file, so a new guard of a value already listed for its file is not reported: a new description != null && <span>{description}</span> in Item.tsx passes under CI=true. Suggest "a check of a value not already listed for its file is reported", or keying the baseline by count.
| */ | ||
|
|
||
| import fs from 'node:fs'; | ||
| import path from 'node:path'; |
There was a problem hiding this comment.
This script matches no-internal-barrel-import.baseline.mjs line for line apart from the rule id, lint targets and message regex. A shared runner taking those as parameters would keep a future fix to the --check flow from landing in only one of them.
| * The boolean test is what marks the value as a rendered node, so these are | ||
| * flagged even outside JSX (architecture:react-component-runtime: a shared | ||
| * concern is extended in its owner and called once). |
There was a problem hiding this comment.
The current architecture:react-component-runtime record has no claim that a shared concern is extended in its owner and called once; that is INV16 in the open #9139, and it covers effects, listeners and trackers. Pointing to utils/isRenderable.ts instead would keep the reference accurate.
PR Analysis Report
📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: The accessibility audit did not run on this pull request. It runs after merges to Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
… share the baseline runner - A chain that also tests the value against another type (typeof x === 'number', isValidElement, Array.isArray, instanceof) is a type switch, not isRenderable by hand: protocol, semanticFence and AppShell's mobileNav check are no longer reported. - The baseline lists each value once per check, so one more check of a listed value in that file is reported; regenerated: 37 checks in 24 files. - rule-baseline.mjs runs the write and --check flow for both this rule's and no-internal-barrel-import's baseline script. - The header points at isRenderable.ts instead of a claim not yet current.
There was a problem hiding this comment.
Changes requested: the hand-written isRenderable check is close, but one type switch still gets through, and the PR carries unrelated formatting.
AppShell.tsx:556-559(mobileNav != null && mobileNav !== false && (isValidElement(mobileNav) || typeof mobileNav === 'string')) is still reported because its type tests sit in a nested group. Following the advice would treat aMobileNavConfigobject as a node, so please treat a grouped type test as a type switch, add that exact shape as a valid test, and regenerate the baseline.- The Prettier-only changes to nine unrelated rule test files (
docblock-example-format.test.mjs,no-raw-color.test.mjsand others) belong in a separate PR. - Minor:
label != null && typeof label !== 'boolean' && <span>{label}</span>is reported twice. Skipping the JSX-guard report once the hand-written check has fired on the same node would fix it.
[Automated review]
| * (`typeof x === 'string'`, `isValidElement(x)`, `Array.isArray(x)`, | ||
| * `x instanceof Y`), negated or not; otherwise null. | ||
| */ | ||
| function otherTypeTestValue(node) { |
There was a problem hiding this comment.
This only reads direct operands, so a grouped type test gets past it. AppShell.tsx:556-559 (mobileNav != null && mobileNav !== false && (isValidElement(mobileNav) || typeof mobileNav === 'string')) is reported as isRenderable by hand, although the README says type switches are left alone. Following the advice would make a MobileNavConfig object count as a node. Treating a parenthesised group of type tests on the value as a type test, plus a valid test with AppShell's exact shape, would clear it and drop the baseline entry.
| return { | ||
| // `{value != null && <El>{value}</El>}` | ||
| LogicalExpression(node) { | ||
| checkHandRolled(node); |
There was a problem hiding this comment.
When the hand-written chain also guards JSX (label != null && typeof label !== 'boolean' && <span>{label}</span>), this node gets two errors, handRolledRenderable and then nullishGuard, and spends two baseline allowances. Skipping the JSX-guard path once checkHandRolled has reported this node would avoid it.
| // Pattern 1: @example on its own line, ``` on next line | ||
| { | ||
| code: ` | ||
| ruleTester.run('docblock-example-format', docblockExampleFormatRule, { |
There was a problem hiding this comment.
This file and eight other rule test files change only by a Prettier pass unrelated to this rule, and no CI step checks .mjs formatting. Please move that reformat to its own PR.
…eport per chain; drop an unrelated reformat - A parenthesised group of type tests on the value ((isValidElement(x) || typeof x === 'string'), AppShell's mobileNav) marks a type switch too; baseline regenerated: 36 checks in 23 files. - A hand-written check that also guards JSX is reported once and spends one baseline allowance. - Restores nine rule test files a Prettier pass had reformatted.
There was a problem hiding this comment.
Approval recommended: the grouped isValidElement(…) || typeof … === 'string' switch in AppShell is now left alone, a hand-written check that also guards JSX is reported once, and the unrelated reformat is gone. Two optional follow-ups: a false | config option check such as scrollLock == null || scrollLock === false is also reported as isRenderable by hand (worth listing under the rule's known limitations), and running both baseline --check scripts in the lint job would keep allowances from outliving migrated checks, as has already happened to the barrel-import baseline.
[Automated review]
| const boolean = | ||
| parseBooleanTypeof(operand, equal) ?? | ||
| parseLiteralComparison(operand, equal, new Set([true, false])); |
There was a problem hiding this comment.
A bare === false / !== false counts as the boolean test here, so a false | config option check such as scrollLock == null || scrollLock === false is reported as isRenderable by hand, and isRenderable will not accept the config object. DropdownMenuGroup, ChatLayout and ChatMessageList use the same literal for real slots, so listing this beside the bare-assignment case under "Known limitations" (rewrite as !scrollLock) may be the simplest fix. Non-blocking.
Maintainer impact
A contributor who decides whether a slot renders by writing out the checks (
label !== '' && label != null && typeof label !== 'boolean', as #8236 did in Badge) gets no lint signal.@astryx/no-nullish-jsx-guardonly looks at bare!= nullJSX guards, and it's a warning even in strict mode, so neither form ever fails a pull request. The copies drift: one includes'', another forgetstrue, and slots disagree about what renders nothing.Intended invariant
A slot's presence is decided by
isRenderablefrom@astryxdesign/core/utils, the one shared definition (null, undefined, true, false and''render nothing;0renders).architecture:react-component-runtimeINV16, in #9139, states the general rule: a shared concern is extended in its owner and called once.Change
''and for a boolean (typeof x !== 'boolean',x !== false,x !== true), anywhere: an assignment, a condition or a return. It also catches the negatedx == null || typeof x === 'boolean'. The boolean test marks the value as a rendered node, so the chain isisRenderableby hand. A presence test without a boolean test (value != null && value !== '') stays valid, and so doesisRenderable.tsitself.{baseline, root}, the same shape as@astryx/no-internal-barrel-import, for both the new check and the existing JSX-guard check.eslint.config.jsenforces the rule in core and richtext at the strict severity, withno-nullish-jsx-guard.baseline.json.no-nullish-jsx-guard.baseline.mjswrites the baseline, and--checkverifies it.Evidence
!= nullJSX guard is only a warning in CI.x != null && typeof x !== 'boolean'added to Avatar.tsx fails strict lint ("writes outisRenderable(x)by hand"). Strict lint overpackages/core/srcandpackages/richtext/srcreports no errors with the baseline, and--checkpasses on main.Scope
isRenderableis the follow-up fix PR.Testing
internal/eslint-plugin-astryx/no-nullish-jsx-guard.test.mjs:isRenderable, presence without a boolean test, different values in the two tests, the utility's own file, and baselined values for both checks.eslint.config.js: 0 errors. The baseline--checkpasses.