From 7858ab3a15fdf384e2ac65b4fef33766b4267b0c Mon Sep 17 00:00:00 2001 From: David Nichols Date: Tue, 6 Oct 2026 20:39:17 +0200 Subject: [PATCH 1/6] =?UTF-8?q?fix(styled):=20styling=20props=20no=20longe?= =?UTF-8?q?r=20reach=20the=20DOM=20=E2=80=94=200.77.7?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit React warned about Reqore's own styling props on DOM elements ("React does not recognize the `hasIcon` prop", "Received `true` for a non-boolean attribute `fill`"), and some were written out as attributes (`theme="[object Object]"`, `color="#454545"` on buttons). Two gaps let them through. A styled DOM tag filters props with is-prop-valid, which keeps every name that is an attribute somewhere: `fill`, `wrap`, `color`, `width`, `opacity`, `direction`, `offset`, ... And a styled component rendered `as` another component (react-aria's DateInput, re-resizable, animated.div, radix, a router link) forwards everything, and that component writes it onto its own element. The bare DatePicker hit the second: ReqoreInput handed its styled input's flags to DateInput, whose Group put them on its div. The styled components now name the styling props they keep back, with the existing omitStyleProps helper. StyledEffect keeps back `color`, `effect`, `intent`, `isText` and `theme`, so every component built on it inherits that. The props ReqoreControlGroup hands its children are one list, REQORE_CONTROL_GROUP_CHILD_PROPS, checked against what the group actually hands out. ReqoreTooltipComponent no longer passes `tooltip` to a DOM element, and ReqoreTree drops its unused `mode`. Every story was rendered and the props each element received were checked; the test renders the components that leaked, reading what React handed each element rather than relying on React's warnings, which come once per prop name per page. --- __tests__/domPropLeaks.test.tsx | 427 ++++++++++++++++++++++ package.json | 2 +- src/components/Button/index.tsx | 7 +- src/components/Checkbox/index.tsx | 11 +- src/components/Collection/index.tsx | 5 +- src/components/Collection/item.tsx | 5 +- src/components/ControlGroup/index.tsx | 3 + src/components/ControlGroup/item.tsx | 17 +- src/components/DatePicker/index.tsx | 8 +- src/components/Effect/index.tsx | 15 +- src/components/Icon/index.tsx | 2 + src/components/Input/index.tsx | 40 +- src/components/Menu/divider.tsx | 7 +- src/components/Menu/index.tsx | 25 +- src/components/Navbar/divider.tsx | 6 +- src/components/Navbar/group.tsx | 6 +- src/components/Navbar/index.tsx | 6 +- src/components/Navbar/item.tsx | 6 +- src/components/Notifications/styles.tsx | 5 +- src/components/Panel/index.tsx | 17 +- src/components/Paragraph/index.tsx | 6 +- src/components/Slider/index.tsx | 7 +- src/components/Spacer/index.tsx | 15 +- src/components/Span/index.tsx | 7 +- src/components/Table/cell.tsx | 5 +- src/components/Table/header.tsx | 5 +- src/components/Tabs/index.tsx | 5 +- src/components/Tabs/item.tsx | 5 +- src/components/Tabs/list.tsx | 5 +- src/components/Tag/index.tsx | 11 +- src/components/Timeline/index.tsx | 27 +- src/components/TooltipComponent/index.tsx | 5 +- src/components/Tree/index.tsx | 2 + src/helpers/styled.ts | 35 ++ src/styles.ts | 18 +- 35 files changed, 716 insertions(+), 62 deletions(-) create mode 100644 __tests__/domPropLeaks.test.tsx diff --git a/__tests__/domPropLeaks.test.tsx b/__tests__/domPropLeaks.test.tsx new file mode 100644 index 00000000..09468b82 --- /dev/null +++ b/__tests__/domPropLeaks.test.tsx @@ -0,0 +1,427 @@ +import isPropValid from '@emotion/is-prop-valid'; +import { render } from '@testing-library/react'; +import { ReactElement } from 'react'; +import { + DatePicker, + ReqoreButton, + ReqoreCheckbox, + ReqoreCollection, + ReqoreContent, + ReqoreControlGroup, + ReqoreHeader, + ReqoreInput, + ReqoreKeyValueTable, + ReqoreLayoutContent, + ReqoreLink, + ReqoreMenu, + ReqoreMenuItem, + ReqoreMessage, + ReqoreNavbarGroup, + ReqoreNavbarItem, + ReqoreP, + ReqorePanel, + ReqoreSlider, + ReqoreSpacer, + ReqoreTabs, + ReqoreTabsContent, + ReqoreTag, + ReqoreTimeline, + ReqoreTree, + ReqoreUIProvider, +} from '../src'; +import { REQORE_CONTROL_GROUP_CHILD_PROPS } from '../src/helpers/styled'; + +/** + * Styling props must not reach the DOM. + * + * Reqore styles its components with props (`fill`, `wrap`, `color`, `opacity`, `theme`, …), and + * styled-components decides which of them the rendered element gets. A DOM tag only gets what + * looks like an HTML attribute, which still lets through the names that ARE attributes somewhere + * (`fill`, `wrap`, `color`, `width`, `direction` …); a component given as `as` (`animated.div`, + * re-resizable, react-aria's `DateInput`, a router link) gets everything and writes it onto its + * own element. React then warns ("React does not recognize the `hasIcon` prop", "Received `true` + * for a non-boolean attribute `fill`") or silently writes `theme="[object Object]"`. + * + * React only warns about a prop name ONCE per page, so a console spy cannot tell the second leak + * of `fill` from the first. This test reads what React actually handed each element instead: + * React 18 keeps an element's current props on the node, under a `__reactProps$` key. + */ + +/** Attributes that exist, but not on the HTML elements a Reqore component renders. */ +const NOT_ON_HTML: Record = { + // SVG presentation attributes. + fill: [], + opacity: [], + direction: [], + offset: [], + spacing: [], + // Legacy `` only. + color: [], + wrap: ['textarea'], + width: ['img', 'canvas', 'video', 'iframe', 'input', 'embed', 'object', 'col', 'colgroup'], + height: ['img', 'canvas', 'video', 'iframe', 'input', 'embed', 'object'], + type: [ + 'input', + 'button', + 'ol', + 'li', + 'a', + 'source', + 'embed', + 'object', + 'script', + 'style', + 'link', + ], + value: [ + 'input', + 'button', + 'select', + 'textarea', + 'option', + 'li', + 'meter', + 'progress', + 'param', + 'data', + 'output', + ], +}; + +const SVG_NAMESPACE = 'http://www.w3.org/2000/svg'; + +const getReactProps = (element: Element): Record | undefined => { + const key = Object.keys(element).find((name) => name.startsWith('__reactProps$')); + + return key ? (element as unknown as Record>)[key] : undefined; +}; + +const describeElement = (element: Element): string => { + const reqoreClass = Array.from(element.classList).find((name) => name.startsWith('reqore-')); + + return `<${element.tagName.toLowerCase()}${reqoreClass ? `.${reqoreClass}` : ''}>`; +}; + +/** Every non-DOM prop React was handed for an element of the document, as ` prop`. */ +const findLeakedProps = (): string[] => { + const leaks: string[] = []; + + document.body.querySelectorAll('*').forEach((element) => { + const props = getReactProps(element); + + if (!props || element.namespaceURI === SVG_NAMESPACE) { + return; + } + + const tag = element.tagName.toLowerCase(); + + Object.entries(props).forEach(([prop, value]) => { + // React renders nothing for an absent value, and says nothing about it either. + if (value === undefined || value === null) { + return; + } + + if (prop === 'children' || prop.startsWith('data-') || prop.startsWith('aria-')) { + return; + } + + const allowedOn = NOT_ON_HTML[prop]; + + if (!isPropValid(prop) || (allowedOn && !allowedOn.includes(tag))) { + leaks.push(`${describeElement(element)} ${prop}`); + } + }); + }); + + return [...new Set(leaks)].sort(); +}; + +const REACT_DOM_PROP_WARNINGS = [ + /does not recognize the `/, + /for a non-boolean attribute/, + /Invalid value for prop/, + /Unknown event handler property/, + /Received NaN for the/, +]; + +const formatConsoleCall = ([format, ...args]: unknown[]): string => { + let index = 0; + + return typeof format === 'string' + ? format.replace(/%s/g, () => String(args[index++])) + : String(format); +}; + +const getReactDomPropWarnings = (): string[] => + vi + .mocked(console.error) + .mock.calls.map(formatConsoleCall) + .filter((message) => REACT_DOM_PROP_WARNINGS.some((pattern) => pattern.test(message))); + +const renderInProvider = (element: ReactElement) => + render( + + + {element} + + + ); + +/** A router's link: it writes every prop it does not know onto its ``. */ +const RouterLinkStub = ({ to, children, ...rest }: any) => ( + + {children} + +); + +// `fill` is not a checkbox prop, but a containing control group hands it to every child, and +// consumers pass it the same way. +const FILL = { fill: true } as any; + +const CASES: [string, () => ReactElement][] = [ + ['a bare DatePicker', () => ], + [ + 'a minimal pill DatePicker with time', + () => , + ], + [ + 'filling Tabs', + () => ( + + One + Two + + ), + ], + [ + 'a filling Collection with a height', + () => ( + + ), + ], + ['a Checkbox given `fill`', () => ], + [ + 'a filling ControlGroup of Tag, Paragraph, Input, Checkbox and Button', + () => ( + + + Paragraph + + + + Button + + + ), + ], + [ + 'a Slider in a filling ControlGroup', + () => ( + + + + ), + ], + [ + 'a resizable Menu with a width', + () => ( + + + + ), + ], + [ + 'a Link rendered as a router link', + () => ( + + Go to issues + + ), + ], + [ + 'a Message with a tooltip', + () => ( + + Message + + ), + ], + [ + 'a Timeline with tooltips', + () => ( + + ), + ], + [ + 'a Navbar', + () => ( + + + Item + + + ), + ], + ['a Tree given a `mode`', () => ], + [ + 'a Panel with an opacity and bottom actions', + () => ( + + Content + + ), + ], + [ + 'a resizable, rounded, interactive Panel', + () => ( + + Content + + ), + ], + ['a KeyValueTable', () => ], + ['a Spacer', () => ], +]; + +describe('no styling prop reaches the DOM', () => { + const originalResizeObserver = globalThis.ResizeObserver; + + beforeAll(() => { + // jsdom has none; the slider measures its thumbs with one. + globalThis.ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + } as unknown as typeof ResizeObserver; + }); + + afterAll(() => { + globalThis.ResizeObserver = originalResizeObserver; + }); + + beforeEach(() => { + vi.mocked(console.error).mockClear(); + }); + + test.each(CASES)('%s', (_name, element) => { + renderInProvider(element()); + + expect(findLeakedProps()).toEqual([]); + expect(getReactDomPropWarnings()).toEqual([]); + }); +}); + +describe('the leak detector', () => { + // Without these, a detector that found nothing would pass every case above. + test('finds a styling flag a DOM element was given', () => { + renderInProvider( +
+ ); + + expect(findLeakedProps()).toEqual([ + ' fill', + ' hasIcon', + ' width', + ]); + }); + + test('leaves real attributes, data and aria attributes, and SVG presentation alone', () => { + renderInProvider( +
+