Skip to content
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.

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

feat: React 19 support #2349

Draft
wants to merge 25 commits into
base: next
Choose a base branch
from

Conversation

CodyJasonBennett
Copy link
Member

@CodyJasonBennett CodyJasonBennett commented Jan 2, 2025

Why

Adds forward support for React 19 and R3F v9.

What

Upstream breaking changes are limited to types, and I opt to remove deprecated patterns where able:

  • useRef<T>() -> useRef<T>(null)
  • (ambient) JSX -> React.JSX
  • MutableRefObject<T> -> RefObject<T> (useRef and RefObject are bugged as immutable in 18.3 types)
  • PropsWithRef -> T (omitted; needs indirection for React 18 + 19 support)
  • (R3F) JSX.IntrinsicElements -> ThreeElements (omitted; this is not backwards compatible to R3F v8.0)

Checklist

  • Documentation updated
  • Demo added
  • Ready to be merged

Copy link

changeset-bot bot commented Jan 2, 2025

⚠️ No Changeset found

Latest commit: 3aaa957

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jan 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
react-spring ✅ Ready (Inspect) Visit Preview Feb 21, 2025 6:41pm

Comment on lines 21 to 23
const host = createHost(primitives, {
// @ts-expect-error r3f related
applyAnimatedValues: applyProps,
})
Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably be a proxy since it is valid to extend the JSX interface beyond what the THREE namespace contains.

@CodyJasonBennett CodyJasonBennett linked an issue Jan 2, 2025 that may be closed by this pull request
5 tasks
This used to be pulled in by R3F, but we vendor it upstream as of late. Even though we intend to later migrate changes, it's best to not depend on transient dependencies.
@joshuaellis
Copy link
Member

@CodyJasonBennett if you can rebase, i've hopefully fixed the issue with the CI :)

@CodyJasonBennett CodyJasonBennett changed the title feat!: React 19 support feat: React 19 support Jan 8, 2025
@CodyJasonBennett

This comment was marked as resolved.

@CodyJasonBennett
Copy link
Member Author

Test runners are supposed to be calling and awaiting act, and many tests are relying on legacy blocking behavior (related: testing-library/react-testing-library#1216).

I'm not sure how to test React 19 here without rewriting the test suite or its instrumentation. Maybe an e2e test could work? Ideally CI could test a matrix of types and implementation.

@ideal-railroad
Copy link

ideal-railroad commented Feb 9, 2025

Test runners are supposed to be calling and awaiting act, and many tests are relying on legacy blocking behavior (related: testing-library/react-testing-library#1216).

I've gone through your branch, and I think I've discovered the problems with act(). act() isn't a suitable substitute for batchedUpdates, because a batched update set can end without completing the layout effects caused by that update. I propose this fix:

packages/core/test/setup.ts L52:

 beforeEach(() => {
   isRunning = true
   frameCache = new WeakMap()
   frameLoop.clear()
   raf.clear()
 
   global.mockRaf = createMockRaf()
   Globals.assign({
     now: global.mockRaf.now,
     requestAnimationFrame: global.mockRaf.raf,
     colors,
     skipAnimation: false,
-    // This lets our useTransition hook force its component
-    // to update from within an "onRest" handler.
-    batchedUpdates: act,
   })
 })

packages/core/test/setup.ts L142 inside global.advanceUntil():

-    jest.advanceTimersByTime(1000 / 60)
+    await act(() => jest.advanceTimersByTimeAsync(1000 / 60))
     global.mockRaf.step()
 
     // Stop observing after the frame is processed.
     for (const value of values) {
       removeFluidObserver(value, frameObserver)
     }
 
     // Ensure pending effects are flushed.
-    flushMicroTasks()
+    await act(() => flushMicroTasks())

That seems to solve the warning.

@ideal-railroad
Copy link

The useSprings() error seems to come from the updates variable not surviving until useIsomorphicLayoutEffect() runs, possibly from react-dev double-running things to catch bugs like this. Wrapping it in a ref and making it clean up after itself seems to do the trick there.

// Apply updates created during render.
const update = updates.current[i]
if (update) {
  // Update the injected ref if needed.
  replaceRef(ctrl, update.ref)

  // When an injected ref exists, the update is postponed
  // until the ref has its `start` method called.
  if (ctrl.ref) {
    ctrl.queue.push(update)
  } else {
    ctrl.start(update)
  }
  
  updates.current[i] = null
}

@Emiliano-Bucci
Copy link

Is there an ETA on this one? :)

@kierancap
Copy link

kierancap commented Mar 20, 2025

I was able to get all the SpringContext related tests to pass by refactoring the context into an standard React Context structure. I'm not totally familiar with ALL the usages of the context in this codebase (but the tests do pass, which with good coverage should indicate feasibility) - So there may have been a valid reason for the Context to be structured as it was.

Anyway... this code works for the Context setup, and like i said, the tests pass :).

import * as React from 'react'
import { useContext, PropsWithChildren } from 'react'

/**
 * This context affects all new and existing `SpringValue` objects
 * created with the hook API or the renderprops API.
 */
export interface SpringContext {
  /** Pause all new and existing animations. */
  pause?: boolean
  /** Force all new and existing animations to be immediate. */
  immediate?: boolean
}

export const SpringContext = React.createContext<SpringContext>({
  pause: false,
  immediate: false,
})

export const SpringContextProvider = ({
  children,
  ...props
}: PropsWithChildren<SpringContext>) => {
  const inherited = useContext(SpringContext)

  // Inherited values are dominant when truthy.
  const pause = props.pause ?? inherited.pause ?? false
  const immediate = props.immediate ?? inherited.immediate ?? false

  // Memoize the context to avoid unwanted renders.
  const contextValue = React.useMemo(
    () => ({ pause, immediate }),
    [pause, immediate]
  )
  return (
    <SpringContext.Provider value={contextValue}>
      {children}
    </SpringContext.Provider>
  )
}

Granted some other files were needed to be refactored to allow for this change. Directly ingesting the Context AND it's Provider into one exported variable seemed... strange - Hence why there were many Typescript and tests failing (Assumption on my end). So splitting them out as is standard for Context, seemed to alleviate the problem.

If there's a valid reason for the Context to be as... strange... as it is, then another solution will be required. Seeing as this PR and the issue it came from is getting stale, I thought i may as well chip in.

@kierancap
Copy link

ALSO - On the testing side of this - Once the context had been "fixed". There was no need for the batchedUpdates: act to be changed in the test setup. All my tests pass locally with just the useRef changes and the Context Refactor

@kierancap kierancap mentioned this pull request Mar 20, 2025
4 tasks
@kierancap
Copy link

As this PR has literally had 0 movement in months - I've made my own PR #2363. The new PR has the changes i mentioned above additionally.

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.

React 19 support
5 participants