Draft
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tracking PR for the React 19 console warnings
@buildcanada/charts@0.3.9emits 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.mdso 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
Accessing element.ref was removed in React 19mobx-react@7.6'smakeClassComponentObserverReact does not recognize the 'lineHeight' prop on a DOM elementtextPropsspread atVerticalColorLegend.tsx:193and similar sites; lands on the<text>SVG element viaMarkdownTextWrap.tsx:753/TextWrap.tsx:330flushSync was called from inside a lifecycle methodgrapher/core/Grapher.tsx:596(IntersectionObserver callback during initial commit)flushSyncis load-bearing for a Safari render bug and forMapTooltip.test.tsx; deferring withqueueMicrotaskregresses the testEncountered two children with the same key, '0'<tspan>lists inTextWrap/MarkdownTextWrapWhy the obvious fix to #1 doesn't work
Bumping
mobx-reactfrom^7.6to^9.2immediately produces 95 test failures with errors of the form:mobx-react 9 strictly enforces what
CLAUDE.mdalready warns against — "Don't use@computedon getters that accessthis.props— props aren't observable in mobx-react" — but the codebase has ~170 sites that violate this:Every
@computed get x() { return this.props.x }getter needs to either lose the@computeddecorator or stop readingthis.propsdirectly. That's its own PR (or several).Why the obvious fix to #4 doesn't work either
Wrapping the call in
queueMicrotaskto escape the active commit phase compiles and stops the warning, butsrc/grapher/mapCharts/MapTooltip.test.tsxthen fails withIceland path should exist— the test runsfireEvent.mouseEnterimmediately after mount, expecting the chart to have rendered synchronously. The Safari bug fix thisflushSyncwas added for also depends on synchronous behavior. Needs a coordinated change.Suggested order of attack
@computedgetters that only forwardthis.props.Xto plain getters (the memoization isn't doing useful work when the input isn't observable). Then bumpmobx-reactpeer to^9.2.0. This single PR removes warnings 1 and 2.textPropscallers and split CSS-shaped fields into a nestedstyle: {...}. Tighten thetextPropstype so thelineHeight-into-DOM leak can't recur.<tspan>lists.Grapher.tsx:596is still needed against current Safari, and either drop theflushSyncor 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.9react@19.2.3,react-dom@19.2.3next@16.1.6(Turbopack)mobx@6.15.3andmobx-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
/dashboardroute during the integration in #7.Generated by Claude Code