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

[Slot] Access ref from props else fallback to element.ref #2811

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chungweileong94
Copy link

Description

fix #2769

Starting from the React 18.3.0 canary, accessing the ref via element.ref will throw a warning, and element.ref will be removed in the future React release.

Instead, we should access the ref from props. To maintain the backward compatibility, we can do children.props.ref ?? children.ref.

@ZipBrandon
Copy link

Just chiming in to say that I had an issue with Portal when used in conjunction with framer-motion in the React canary releases since 19.0.0-canary-7a2609eed-20240403 and that this was the fix. Thank you.

@alaney2
Copy link

alaney2 commented Apr 10, 2024

Thanks for fixing this

@borispoehland
Copy link

Thank you!

@tillka
Copy link

tillka commented Apr 26, 2024

Is there a way to fix this manually / locally until the official fix is out? The 7x 'element.ref' warnings I am getting on each page refresh are very annoying.

@juliusmarminge
Copy link

I also had to patch @radix-ui/react-presence to get rid of all warnings:

CleanShot 2024-04-26 at 21 31 55@2x

@chungweileong94
Copy link
Author

chungweileong94 commented Apr 26, 2024

I also had to patch @radix-ui/react-presence to get rid of all warnings:

CleanShot 2024-04-26 at 21 31 55@2x

Ah thx, wasn’t able to find the others, will include this as well.

@chungweileong94
Copy link
Author

@juliusmarminge I applied the same fix to @radix-ui/react-presence too.

Copy link

@juliusmarminge juliusmarminge left a comment

Choose a reason for hiding this comment

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

LGTM

@tungv
Copy link

tungv commented May 6, 2024

Just a gentle reminder that this PR being merged and released is so valuable for a lot of people. Thank you.

const ref = useComposedRefs(presence.ref, (child as any).ref);
// Accessing the ref from props, else fallback to element.ref
// https://github.com/facebook/react/pull/28348
const childrenRef = child.props.ref ?? (child as any).ref;
Copy link

Choose a reason for hiding this comment

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

It's fine to access child.ref conditionally. React only warns when accessing child.ref iff a ref prop is actually passed.

Copy link

@nicksrandall nicksrandall left a comment

Choose a reason for hiding this comment

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

LGTM

@Brenndoerfer
Copy link

Would be great to get an approval from one of the code owners on this.. it also impacts Shadcn/ui

@paplco
Copy link

paplco commented May 27, 2024

hey @andy-hook @benoitgrelard

hoping this gets approved soon. thanks!

@sannajammeh
Copy link

sannajammeh commented May 28, 2024

As this is taking a little while, here's how to suppress the annoying logs until its merged:

_supressLogs.ts

// TODO - delete when https://github.com/radix-ui/primitives/pull/2811 gets merged 
const prevConsoleError = console.error;
const prevConsoleWarn = console.warn;

console.error = (...args) => {
  if (args[0].includes("Warning: Accessing element.ref")) {
    return;
  }

  prevConsoleError(...args);
};

console.warn = (...args) => {
  if (args[0].includes("Warning: Accessing element.ref")) {
    return;
  }

  prevConsoleWarn(...args);
};

Import this globally at your app root.
For Next.js this would be

Root layout.tsx

import "./_supressLogs";
// Your root layout

And Root client provider file _Providers.tsx

"use client";

import "./_supressLogs";
// Your provider

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.

[Slot] React 18.3.0 canary, ref is now a regular prop