Skip to content

React 19 compatibility cleanup (tracking)#8

Draft
xrendan wants to merge 1 commit intomasterfrom
claude/react-19-compatibility
Draft

React 19 compatibility cleanup (tracking)#8
xrendan wants to merge 1 commit intomasterfrom
claude/react-19-compatibility

Conversation

@xrendan
Copy link
Copy Markdown
Member

@xrendan xrendan commented May 8, 2026

Summary

Tracking PR for the React 19 console warnings @buildcanada/charts@0.3.9 emits when mounted in a React 19 host (Next.js 16 / React 19.2.3, surfaced while integrating into BuildCanada/tradingpost). The chart renders and is fully interactive, but five separate warnings show up; this PR captures what they are, where they originate, and why I didn't just fix them in this PR — most need either a coordinated test/runtime change or a sweeping refactor.

The actual diff is just packages/charts/REACT_19_TODO.md so the audit trail lives next to the code. Treat this as a draft / discussion vehicle: the maintainer can split the items into separate PRs, prioritize, or close.

Findings

# Warning Source Status
1 unmet peer `react@"^16.8.0 ^17
2 Accessing element.ref was removed in React 19 inside mobx-react@7.6's makeClassComponentObserver downstream of (1)
3 React does not recognize the 'lineHeight' prop on a DOM element textProps spread at VerticalColorLegend.tsx:193 and similar sites; lands on the <text> SVG element via MarkdownTextWrap.tsx:753 / TextWrap.tsx:330 not fixed here — needs a typed split between SVG attrs and CSS-shaped fields
4 flushSync was called from inside a lifecycle method grapher/core/Grapher.tsx:596 (IntersectionObserver callback during initial commit) not fixed here — the flushSync is load-bearing for a Safari render bug and for MapTooltip.test.tsx; deferring with queueMicrotask regresses the test
5 Encountered two children with the same key, '0' index-only keys on <tspan> lists in TextWrap / MarkdownTextWrap not fixed here — needs a per-context key prefix

Why the obvious fix to #1 doesn't work

Bumping mobx-react from ^7.6 to ^9.2 immediately produces 95 test failures with errors of the form:

Error: [mobx-react] Cannot read "DataTable.props" in a reactive context, as it isn't observable.
                  Please use component lifecycle method to copy the value into a local observable first.
  at DataTable.get [as props] node_modules/mobx-react/.../mobxreact.cjs.development.js:337:15
  at DataTable.get manager src/grapher/dataTable/DataTable.tsx:111:21

mobx-react 9 strictly enforces what CLAUDE.md already warns against — "Don't use @computed on getters that access this.props — props aren't observable in mobx-react" — but the codebase has ~170 sites that violate this:

$ grep -rB1 "this\.props" packages/charts/src --include="*.tsx" | grep -c "@computed"
174

Every @computed get x() { return this.props.x } getter needs to either lose the @computed decorator or stop reading this.props directly. That's its own PR (or several).

Why the obvious fix to #4 doesn't work either

Wrapping the call in queueMicrotask to escape the active commit phase compiles and stops the warning, but src/grapher/mapCharts/MapTooltip.test.tsx then fails with Iceland path should exist — the test runs fireEvent.mouseEnter immediately after mount, expecting the chart to have rendered synchronously. The Safari bug fix this flushSync was added for also depends on synchronous behavior. Needs a coordinated change.

Suggested order of attack

  1. Remove OWID and Our World in Data references from codebase #1 / Add @buildcanada/components package with Chromatic visual testing #2 first. Convert the @computed getters that only forward this.props.X to plain getters (the memoization isn't doing useful work when the input isn't observable). Then bump mobx-react peer to ^9.2.0. This single PR removes warnings 1 and 2.
  2. Release v0.2.0: Dialog, PopupForm components and CI publishing #3. Audit textProps callers and split CSS-shaped fields into a nested style: {...}. Tighten the textProps type so the lineHeight-into-DOM leak can't recur.
  3. Fix MobX decorator incompatibility: switch to TC39 stage 3 decorators #5. Replace index keys with content-derived keys on <tspan> lists.
  4. Integrate Build Canada design system #4 last. Re-evaluate whether the Safari workaround at Grapher.tsx:596 is still needed against current Safari, and either drop the flushSync or migrate the test to wait for the deferred render.

Happy to pick up any of these as follow-up PRs — let me know which you'd like prioritized.

Repro context

  • @buildcanada/charts@0.3.9
  • react@19.2.3, react-dom@19.2.3
  • next@16.1.6 (Turbopack)
  • mobx@6.15.3 and mobx-react@7.6.0 (auto-installed peers)

The Grapher renders cleanly in this stack — these are warnings, not errors. Verified end-to-end via headless Chromium against TradingPost's /dashboard route during the integration in #7.


Generated by Claude Code

This is a draft tracking the warnings the charts package emits when
mounted in a React 19 host. The chart still renders and is fully
interactive, but five things would need attention to declare full
React 19 readiness:

1. mobx-react peer pin doesn't include React 19. Bumping to ^9 (the
   version that does) fails ~95 tests because mobx-react 9 strictly
   enforces 'no @computed on getters that read this.props' — which
   CLAUDE.md already warns against, but ~170 sites in this codebase
   currently violate.

2. element.ref deprecation warning, sourced from inside mobx-react
   7.6's class-component observer. Goes away with the mobx-react bump
   from #1.

3. lineHeight prop reaching an SVG <text> element via a textProps
   spread (VerticalColorLegend.tsx:193 et al.). Needs a refactor to
   keep CSS in a nested style={...} and SVG attrs at the top level.

4. flushSync-during-render warning at Grapher.tsx:596. The flushSync
   is load-bearing for a Safari rendering bug AND for
   MapTooltip.test.tsx, so the obvious queueMicrotask wrap regresses
   tests. Needs a coordinated fix.

5. Duplicate key='0' from index-only keys on <tspan> lists in
   TextWrap / MarkdownTextWrap.

I tried (1) and (4) on this branch and reverted both for the reasons
above. (2) is downstream of (1). (3) and (5) need source refactors I
didn't want to land speculatively in a draft. This commit is just the
written-up audit trail so the work can be picked up incrementally.

The full repro context (Next.js 16, React 19.2.3, charts 0.3.9) and
the playwright check that surfaced these is in the TradingPost branch
that integrates the package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants