Skip to content

fix(react19): remove React 19 incompatible APIs and restore compatibility - #1158

Open
kkarthik-eightfold wants to merge 5 commits into
mainfrom
kkarthik/react19-compatibility
Open

fix(react19): remove React 19 incompatible APIs and restore compatibility#1158
kkarthik-eightfold wants to merge 5 commits into
mainfrom
kkarthik/react19-compatibility

Conversation

@kkarthik-eightfold

Copy link
Copy Markdown

SUMMARY:

Makes @eightfold.ai/octuple compatible with React 19 so it can be consumed by Talent Forge, by removing all removed/incompatible React 19 APIs while preserving public APIs and existing behavior.

React 19 removed ReactDOM.render, ReactDOM.unmountComponentAtNode, and ReactDOM.findDOMNode, and moved element refs from a top-level .ref property onto props.ref. These broke Octuple in several places:

  • DialogHelper used ReactDOM.render/unmountComponentAtNode directly, which made the whole @eightfold.ai/octuple barrel import fail under React 19. Migrated to createRoot/root.render/root.unmount from react-dom/client, tracking one root per container id so close() unmounts the right instance. Public show()/showSmall()/showMedium()/close() APIs are unchanged.

  • findDOMNode.ts, SingleObserver.tsx, CSSMotion.tsx, Trigger.tsx, useHeights.tsx all depended on ReactDOM.findDOMNode (directly or via the shared findDOMNode() helper) to resolve a DOM node from a component that couldn't accept a ref. findDOMNode() is now a null-safe passthrough with no ReactDOM dependency. DomWrapper (the SingleObserver/CSSMotion fallback) now resolves the DOM node via a hidden marker node instead, placed after (not before) its children so container.firstChild/:first-child semantics are preserved; the marker is only rendered at all when a direct ref genuinely can't be attached (see canAttachRef in ref.ts), so Table's <td> (which must stay a direct child of <tr>) and other host/forwardRef children get zero extra DOM nodes.

  • Added canAttachRef() and getElementRef() to ref.ts. getElementRef() reads props.ref on React 19 and falls back to the legacy top-level .ref on React 16.8–18 (Octuple's peerDependencies still span that range) — reading props.ref alone silently drops real refs on React <19, which broke Align's forceAlign wiring inside PopupInner until this was corrected. Rolled out across SingleObserver, CSSMotion, Trigger, Align, and useItemRef.

  • react-is@18 doesn't recognize React 19 elements (different $$typeof symbol), and react-is@19 doesn't recognize React <19 elements — react-is versions are generation-locked, not version-range compatible. Since a single bundled react-is version can't correctly serve every React major Octuple supports, toArray.ts's Fragment detection and ref.ts's forwardRef detection now check the relevant symbols directly instead of delegating to react-is. Bumped react-is/@types/react-is to 19.x per the target anyway.

  • @react-spring/web's peerDependencies already allow React 19; left unchanged.

  • Added a global.d.ts ambient declaration for react-dom/client so the build type-checks against this repo's own React 17 devDependency, which predates that module.

GITHUB ISSUE (Open Source Contributors)

N/A

JIRA TASK (Eightfold Employees Only):

N/A

CHANGE TYPE:

  • Bugfix Pull Request
  • Feature Pull Request

TEST COVERAGE:

  • Tests for this change already exist
  • I have added unittests for this change

Added: DialogHelper.test.tsx (mount/unmount via createRoot), findDOMNode.test.ts (null-safety), reactIs.test.tsx (React 19 Fragment detection via toArray), plus rewritten cases in CSSMotion.test.tsx (DOM resolution without findDOMNode). Updated 35 pre-existing snapshots (Select, SkillBlock, DatePicker) that gained the new hidden DomWrapper marker node, after confirming each is the accepted, documented trade-off.

TEST PLAN:

  • yarn typecheck — pass
  • yarn lint — pass
  • yarn test — 225/225 suites, 2657/2679 tests pass (22 pre-existing skips), 441/441 snapshots pass
  • yarn build (rollup) — succeeds, including the react-dom/client import
  • Manually verified no remaining ReactDOM.render/unmountComponentAtNode/ReactDOM.findDOMNode/legacy element.ref usage in src

Known limitations / follow-ups

  • This repo's own devDependencies remain on React 17 (bumping them breaks the 144-file Enzyme-based test suite, which has no official React 18/19 adapter), so DialogHelper.test.tsx uses a virtual jest.mock('react-dom/client', ...) shim backed by React 17's ReactDOM.render/unmountComponentAtNode to exercise the public API against this repo's own toolchain. Real consumers on React 18/19 get the real module. A follow-up to migrate the test suite off Enzyme would let this repo verify against a real React 19 install directly.
  • Found and flagged separately (not fixed here, out of scope): Dialog.tsx accepts a parent prop but never forwards it to BaseDialog, so DialogHelper always portals into document.body regardless of the container it creates. Pre-existing, unrelated to React 19.
  • 6 components (Tree, DirectoryTree, BaseTree, Table, OcTable, plus TreeNode's class component) set .defaultProps on function/forwardRef components. React 19 still supports this but logs a deprecation warning; not addressed here since it doesn't break functionality and wasn't part of the original investigation's scope.

K Karthik and others added 2 commits August 8, 2026 11:58
…lity

React 19 removed ReactDOM.render, ReactDOM.unmountComponentAtNode, and
ReactDOM.findDOMNode, and moved element refs from a top-level `.ref`
property onto `props.ref`. These changes broke Octuple in several places:

- DialogHelper used ReactDOM.render/unmountComponentAtNode directly,
  which fails to import at all under React 19. Migrated to
  createRoot/root.render/root.unmount from react-dom/client, tracking
  one root per container id so close() unmounts the right instance.
  Public show()/showSmall()/showMedium()/close() APIs are unchanged.

- findDOMNode.ts, SingleObserver.tsx, CSSMotion.tsx, Trigger.tsx, and
  useHeights.tsx all depended on ReactDOM.findDOMNode, directly or via
  the shared findDOMNode() helper, to resolve a DOM node from a
  component that couldn't accept a ref. findDOMNode() is now a
  null-safe passthrough with no ReactDOM dependency. DomWrapper
  (SingleObserver/CSSMotion's fallback) now resolves the DOM node via
  a hidden marker node instead, placed after (not before) its
  children so container.firstChild/:first-child semantics are
  preserved for the common case; the marker is only rendered at all
  when a direct ref genuinely can't be attached (see canAttachRef in
  ref.ts), so Table's <td> (which must stay a direct child of <tr>)
  and other host/forwardRef children get zero extra DOM nodes.

- Added canAttachRef() and getElementRef() to ref.ts. getElementRef()
  reads props.ref on React 19 and falls back to the legacy top-level
  .ref on React 16.8-18 (Octuple's peerDependencies still span that
  range) — reading props.ref alone silently drops real refs on
  React <19, which broke Align's forceAlign wiring inside PopupInner
  until this was corrected. Rolled out across SingleObserver, CSSMotion,
  Trigger, Align, and useItemRef.

- react-is@18 doesn't recognize React 19 elements (different $$typeof
  symbol), and react-is@19 doesn't recognize React <19 elements —
  react-is versions are generation-locked, not version-range
  compatible. Since a single bundled react-is version can't correctly
  serve every React major Octuple supports, toArray.ts's Fragment
  detection and ref.ts's forwardRef detection now check the relevant
  symbols directly instead of delegating to react-is. Bumped
  react-is/@types/react-is to 19.x per the target anyway.

- @react-spring/web's peerDependencies already allow React 19; left
  unchanged.

Added regression tests: DialogHelper mount/unmount, findDOMNode null
handling, and React 19 Fragment detection via toArray. Updated 35
existing snapshots that gained the new hidden DomWrapper marker node
(Select, SkillBlock, DatePicker) after confirming each is the expected,
documented trade-off.

Added a global.d.ts ambient declaration for react-dom/client so the
build type-checks against this repo's own React 17 devDependency,
which predates that module.

Note: this repo's own devDependencies remain on React 17 (bumping them
breaks the Enzyme-based test suite, which has no React 18/19 adapter),
so DialogHelper's test uses a virtual jest.mock of react-dom/client
backed by React 17's ReactDOM.render/unmountComponentAtNode. Real
consumers on React 18/19 get the real module.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codesandbox-ci

codesandbox-ci Bot commented Aug 8, 2026

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@codecov

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.21%. Comparing base (fac8548) to head (491ca41).

Files with missing lines Patch % Lines
src/components/Dialog/DialogHelper.tsx 88.00% 3 Missing ⚠️
src/shared/utilities/ref.ts 86.66% 2 Missing ⚠️
...d/ResizeObserver/SingleObserver/SingleObserver.tsx 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1158      +/-   ##
==========================================
+ Coverage   85.07%   85.21%   +0.13%     
==========================================
  Files        1230     1230              
  Lines       21593    21653      +60     
  Branches     8211     8243      +32     
==========================================
+ Hits        18370    18451      +81     
+ Misses       3137     3118      -19     
+ Partials       86       84       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

K Karthik and others added 3 commits August 10, 2026 08:37
DialogHelper.tsx imports createRoot from react-dom/client (added in
React 18) as part of the React 19 compatibility migration. This repo's
own devDependency is still pinned to react-dom@17.0.2 (kept there to
avoid breaking the Enzyme-based test suite, which has no React 18/19
adapter), so that module doesn't exist on disk, and Storybook's
webpack build failed to resolve it: `yarn storybook`/`yarn
build-storybook` couldn't build at all.

Added a Storybook-only shim (.storybook/shims/reactDomClient.js) that
implements just the createRoot API used here, backed by React 17's
ReactDOM.render/unmountComponentAtNode, and wired it in via
NormalModuleReplacementPlugin in .storybook/main.js (a plain
resolve.alias entry gets reset by another addon's webpackFinal before
resolution happens, so the replacement plugin is used instead).

This only affects the local Storybook build/dev server. It doesn't
touch package.json, the rollup library build, Jest, or any shipped
src/ file — real consumers on React 18/19 still resolve the genuine
react-dom/client from their own install.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ed react-is dependency

- Migrate all React 19-unsafe defaultProps usages (Tree, BaseTree,
  ForwardDirectoryTree, ContextTreeNode, Table, OcTable) to parameter
  defaults; class-component defaultProps are left untouched (still
  supported in React 19)
- Fix two second-order regressions surfaced by the migration: BaseTree
  wasn't explicitly forwarding showIcon to OcTree, and
  treeUtil.convertTreeToData() read raw element.props.title without
  rendering, both of which previously relied on defaultProps' implicit
  createElement-time merge
- Remove the unused, duplicated react-is direct dependency (nothing in
  src/ imports it; toArray.ts/ref.ts already do their own $$typeof
  checks); add scripts/verifyReactIsBundle.js (wired into yarn build)
  and reactIsDependency.test.ts as regression protection
- Add scripts/verifyNoFindDOMNode.js (wired into yarn lint) to catch
  any reintroduced ReactDOM.findDOMNode usage
- Add focused regression tests proving actual resolved defaultProps
  behavior (Table/OcTable rowKey+emptyText, Tree/BaseTree/DirectoryTree
  showIcon/selectable/blockNode), including explicit-falsy-override
  cases
- Rename reactIs.test.tsx to toArrayFragmentDetection.test.tsx to match
  what it actually tests
- Add docs/react19-phase1-compatibility-report.md documenting the full
  before/after audit, compatibility matrix, and known gaps (no React 19
  runtime was executed against this code; date-fns/dayjs duplication in
  real consumer installs investigated and documented but not fixed)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

1 participant