Skip to content

fix(ScrollControls): cache ReactDOM root for React 19 compatibility #2557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iamadagostino wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(ScrollControls): cache ReactDOM root for React 19 compatibility #2557

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
493e8c3
fix(ScrollControls): cache ReactDOM root for React 19 compatibility
iamadagostino Oct 13, 2025
ae411a6
refactor(ScrollControls): use proper HTMLDivElementWithR3Root type in…
iamadagostino Oct 23, 2025
bb9316c
Merge branch 'master' into master
iamadagostino Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion src/web/ScrollControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,32 @@ const ScrollHtml: ForwardRefComponent<{ children?: React.ReactNode; style?: Reac
React.useImperativeHandle(ref, () => group.current, [])
const { width, height } = useThree((state) => state.size)
Copy link

@artokun artokun Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you update useThree types to include state.fixed and tag me for review again?

Copy link
Author

@iamadagostino iamadagostino Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The state variable used in the root caching logic (lines 245-264) comes from useScroll() on line 234, which returns ScrollControlsState (already typed with fixed: HTMLDivElement on line 37).
  • This useThree() call only extracts size for layout calculations and is unrelated to the state.fixed property being accessed below.

Copy link

@artokun artokun Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See R247

const fiberState = React.useContext(fiberContext)
const root = React.useMemo(() => ReactDOM.createRoot(state.fixed), [state.fixed])
// Cache a root instance on the fixed DOM element to avoid calling
// ReactDOM.createRoot multiple times on the same container which
// triggers an error in React 19. We store the created root in a
// non-enumerable property on the element so repeated mounts or
// remounts re-use the same root instance.
const root = React.useMemo(() => {
try {
// If a root was previously created on this element, reuse it.
if (state.fixed && (state.fixed as any).__r3_root) return (state.fixed as any).__r3_root
Copy link

@artokun artokun Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I am trying to avoid is this. why is there a __r3_root property on a HTMLDIvElement? Seems odd that we need to typecast this to any.

Copy link
Author

@iamadagostino iamadagostino Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I've replaced the as any typecast with a proper type definition. Created a new type HTMLDivElementWithR3Root (lines 9-11) that explicitly defines the __r3_root property on the HTMLDivElement. This makes it clear that we intentionally store the ReactDOM root instance on this element to cache it across remounts, and TypeScript now knows about this property without needing unsafe casts.

const r = ReactDOM.createRoot(state.fixed)
try {
Object.defineProperty(state.fixed, '__r3_root', {
value: r,
configurable: true,
writable: false,
})
Copy link

@artokun artokun Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify the return type of the useThree lambda, if it's a valtio proxy, you'll need to properly define it in the store. we don't want to be setting properties this way.

Copy link
Author

@iamadagostino iamadagostino Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state.fixed here is from useScroll() (line 234), which returns ScrollControlsState - not from R3F's store. It's a plain HTMLDivElement created via useState (line 68), not a Valtio proxy.
Setting __r3_root on this DOM element doesn't mutate any reactive store state.
The property is defined non-enumerable and is used solely to cache the ReactDOM root instance to prevent React 19's "createRoot already called" error on remount.

Copy link

@artokun artokun Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's clever that you're adding a property on a html element, I just haven't seen this done anywhere else which is why it's confusing me. Can we create a new type that has this modification? like a HTMLDivElementWithR3Root

Copy link
Author

@iamadagostino iamadagostino Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! I created the HTMLDivElementWithR3Root type (lines 9-11) that extends HTMLDivElement with an optional __r3_root?: ReactDOM.Root property. This pattern allows us to cache the root instance on the DOM element itself, preventing React 19's "createRoot already called" error when ScrollHtml remounts. The type makes it explicit and type-safe, while the non-enumerable property definition keeps it hidden from standard DOM inspection. Thanks for pushing for clarity here!

} catch (e) {
// ignore if we can't define property (very unlikely)
;(state.fixed as any).__r3_root = r
}
return r
} catch (e) {
// Fallback: if createRoot fails for any reason, rethrow so devs see it.
throw e
}
}, [state.fixed])
useFrame(() => {
if (state.delta > state.eps) {
group.current.style.transform = `translate3d(${
Expand Down