fix(react19): remove React 19 incompatible APIs and restore compatibility - #1158
Open
kkarthik-eightfold wants to merge 5 commits into
Open
fix(react19): remove React 19 incompatible APIs and restore compatibility#1158kkarthik-eightfold wants to merge 5 commits into
kkarthik-eightfold wants to merge 5 commits into
Conversation
…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>
|
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 Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
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>
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:
Makes
@eightfold.ai/octuplecompatible 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, andReactDOM.findDOMNode, and moved element refs from a top-level.refproperty ontoprops.ref. These broke Octuple in several places:DialogHelperusedReactDOM.render/unmountComponentAtNodedirectly, which made the whole@eightfold.ai/octuplebarrel import fail under React 19. Migrated tocreateRoot/root.render/root.unmountfromreact-dom/client, tracking one root per container id soclose()unmounts the right instance. Publicshow()/showSmall()/showMedium()/close()APIs are unchanged.findDOMNode.ts,SingleObserver.tsx,CSSMotion.tsx,Trigger.tsx,useHeights.tsxall depended onReactDOM.findDOMNode(directly or via the sharedfindDOMNode()helper) to resolve a DOM node from a component that couldn't accept a ref.findDOMNode()is now a null-safe passthrough with noReactDOMdependency.DomWrapper(theSingleObserver/CSSMotionfallback) now resolves the DOM node via a hidden marker node instead, placed after (not before) its children socontainer.firstChild/:first-childsemantics are preserved; the marker is only rendered at all when a direct ref genuinely can't be attached (seecanAttachRefinref.ts), soTable's<td>(which must stay a direct child of<tr>) and other host/forwardRefchildren get zero extra DOM nodes.Added
canAttachRef()andgetElementRef()toref.ts.getElementRef()readsprops.refon React 19 and falls back to the legacy top-level.refon React 16.8–18 (Octuple'speerDependenciesstill span that range) — readingprops.refalone silently drops real refs on React <19, which brokeAlign'sforceAlignwiring insidePopupInneruntil this was corrected. Rolled out acrossSingleObserver,CSSMotion,Trigger,Align, anduseItemRef.react-is@18doesn't recognize React 19 elements (different$$typeofsymbol), andreact-is@19doesn't recognize React <19 elements —react-isversions are generation-locked, not version-range compatible. Since a single bundledreact-isversion can't correctly serve every React major Octuple supports,toArray.ts's Fragment detection andref.ts'sforwardRefdetection now check the relevant symbols directly instead of delegating toreact-is. Bumpedreact-is/@types/react-isto 19.x per the target anyway.@react-spring/web'speerDependenciesalready allow React 19; left unchanged.Added a
global.d.tsambient declaration forreact-dom/clientso 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:
TEST COVERAGE:
Added:
DialogHelper.test.tsx(mount/unmount viacreateRoot),findDOMNode.test.ts(null-safety),reactIs.test.tsx(React 19 Fragment detection viatoArray), plus rewritten cases inCSSMotion.test.tsx(DOM resolution withoutfindDOMNode). Updated 35 pre-existing snapshots (Select, SkillBlock, DatePicker) that gained the new hiddenDomWrappermarker node, after confirming each is the accepted, documented trade-off.TEST PLAN:
yarn typecheck— passyarn lint— passyarn test— 225/225 suites, 2657/2679 tests pass (22 pre-existing skips), 441/441 snapshots passyarn build(rollup) — succeeds, including thereact-dom/clientimportReactDOM.render/unmountComponentAtNode/ReactDOM.findDOMNode/legacyelement.refusage insrcKnown limitations / follow-ups
DialogHelper.test.tsxuses a virtualjest.mock('react-dom/client', ...)shim backed by React 17'sReactDOM.render/unmountComponentAtNodeto 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.Dialog.tsxaccepts aparentprop but never forwards it toBaseDialog, soDialogHelperalways portals intodocument.bodyregardless of the container it creates. Pre-existing, unrelated to React 19.Tree,DirectoryTree,BaseTree,Table,OcTable, plusTreeNode's class component) set.defaultPropson 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.