Skip to content

React 19 compatibility cleanup (tracking) - #8

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

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

Conversation

@xrendan

Copy link
Copy Markdown
Member

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

#WarningSourceStatus
1unmet peer `react@"^16.8.0^17
2Accessing element.ref was removed in React 19inside mobx-react@7.6's makeClassComponentObserverdownstream of (1)
3React does not recognize the 'lineHeight' prop on a DOM elementtextProps spread at VerticalColorLegend.tsx:193 and similar sites; lands on the <text> SVG element via MarkdownTextWrap.tsx:753 / TextWrap.tsx:330not fixed here — needs a typed split between SVG attrs and CSS-shaped fields
4flushSync was called from inside a lifecycle methodgrapher/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
5Encountered two children with the same key, '0'index-only keys on <tspan> lists in TextWrap / MarkdownTextWrapnot 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 freeto 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

@xrendan@claude