diff --git a/packages/charts/REACT_19_TODO.md b/packages/charts/REACT_19_TODO.md new file mode 100644 index 00000000000..2c78f9c2278 --- /dev/null +++ b/packages/charts/REACT_19_TODO.md @@ -0,0 +1,133 @@ +# React 19 compatibility — TODO + +This document tracks console warnings / errors that `@buildcanada/charts` +produces when mounted in a React 19 host (`react@19.x`, `react-dom@19.x`). +The chart still renders and is interactive, but each warning is worth +addressing before declaring full React 19 readiness. + +Measured against `@buildcanada/charts@0.3.9` mounted on Next.js 16 +(Turbopack) with React 19.2.3 and the registry-default peer auto-install +(`mobx@6.15.x`, `mobx-react@7.6.x`). + +## 1. mobx-react peer dep doesn't include React 19 + +**Where:** `packages/charts/package.json` — `peerDependencies.mobx-react: "^7.6.0"`. + +mobx-react 7.6 declares its own peer as `react: "^16.8.0 || ^17 || ^18"`, +so consumers on React 19 always see: + +``` +WARN Issues with peer dependencies found +└─┬ mobx-react 7.6.0 + ├── ✕ unmet peer react@"^16.8.0 || ^17 || ^18": found 19.2.3 + └─┬ mobx-react-lite 3.4.3 + └── ✕ unmet peer react@"^16.8.0 || ^17 || ^18": found 19.2.3 +``` + +mobx-react 9.x adds React 19 to its peer range and supports TC-39 stage +3 decorators natively (matching what `build.ts` now emits). + +**Why this isn't a one-line bump.** Trying `mobx-react ^9.2.0` in this +workspace immediately fails ~95 tests with: + +``` +Error: [mobx-react] Cannot read "DataTable.props" in a reactive context, as it +isn't observable. +``` + +mobx-react 9 strictly enforces what `CLAUDE.md` already warns about +("Don't use `@computed` on getters that access `this.props`"). The +codebase has ~170 `@computed get x() { return this.props.x }` sites +that all need to lose either the `@computed` decorator or the +`this.props` read. Failing test files include `DataTable.test.tsx`, +`ScatterPlotChart.test.ts`, and many more. + +**Suggested approach:** drop `@computed` from getters that only +forward `this.props.X` (since the memoization isn't doing useful work +when the input isn't observable anyway), then bump the peer dep in a +follow-up. + +## 2. `Accessing element.ref was removed in React 19. ref is now a regular prop.` + +**Where:** logged on every render of an `@observer`-decorated class. +Originates inside `mobx-react@7.6`'s `makeClassComponentObserver`, +which reads `element.ref` to forward refs through the observer wrapper. +React 19 removed that accessor and treats `ref` as a regular prop. + +Fixed transitively by issue 1 — mobx-react 9 doesn't read +`element.ref`. + +## 3. `React does not recognize the 'lineHeight' prop on a DOM element.` + +**Where:** somewhere along the SVG `` rendering chain in +`packages/charts/src/components/MarkdownTextWrap/MarkdownTextWrap.tsx` +and/or `packages/charts/src/components/TextWrap/TextWrap.tsx`. Both +spread caller-supplied `textProps` directly onto the `` element +(`MarkdownTextWrap.tsx:753`, `TextWrap.tsx:330`), and at least one +caller (`grapher/legend/VerticalColorLegend.tsx:193`) does: + +```tsx +textProps: { fill: style.color, ...style }, +``` + +That spreads CSS-shaped fields onto the SVG element as DOM +attributes. `LegendTextStyle` only declares `color | fontWeight | +opacity`, but the chart-level style configs that feed in can extend +that, and at runtime React 19 sees `lineHeight` reach a DOM node and +warns. (React 18 tolerated it.) + +**Suggested fix:** keep `textProps` strictly typed as +`React.SVGProps`, and put any CSS-shaped fields +(`color → fill` already, plus `lineHeight`, `fontWeight`, `opacity`, +…) under a nested `style: { … }` rather than spreading them at the +top level. + +## 4. `flushSync was called from inside a lifecycle method.` + +**Where:** `packages/charts/src/grapher/core/Grapher.tsx:596`, inside +the `IntersectionObserver` callback set up in +`setUpIntersectionObserver`: + +```ts +flushSync(action(() => { this.hasBeenVisible = true })) +``` + +The `flushSync` exists to dodge a Safari bug where the initial render +uses stale bounds; it predates React 19. React 19 logs a warning when +`flushSync` fires while React is still in a render/commit phase, which +this can do if the `IntersectionObserver` entry is delivered in the +same task as the mount commit. + +**Why this isn't a quick fix.** Wrapping in `queueMicrotask(() => +flushSync(...))` silences the warning but breaks +`src/grapher/mapCharts/MapTooltip.test.tsx` (the test depends on the +chart being rendered synchronously after the observer fires, asserting +`'path[data-feature-id="Iceland"]'` is present). The Safari workaround +also depends on this being synchronous. A real fix needs a coordinated +change: either move the test to wait for the deferred render, or +replace `flushSync` with a different mechanism that the Safari bug +also accepts. + +The `flushSync` use in +`packages/charts/src/components/reactUtil.ts:16` is fine — it's +outside the render path. + +## 5. `Encountered two children with the same key, '0'.` + +**Where:** observed during `MapTooltip` render and around `` +lists. Index-only keys at `TextWrap.tsx:344` / `MarkdownTextWrap.tsx` +likely collide when multiple list nodes are rendered as siblings of +the same parent. + +**Suggested fix:** prefix the index with the surrounding context +(line content hash, parent id, …) so keys don't collide across +sibling lists. + +## Out of scope + +- The `Image with src "..." has either width or height modified, but + not the other` warning observed when integrating the charts package + into TradingPost is from TradingPost's own navbar, not charts. +- The ` / / is unrecognized in this browser` stderr + noise during axis tests is happy-dom's test environment, not React + 19.