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

[compiler] Treat ref-like named objects as refs #29916

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

gsathya
Copy link
Contributor

@gsathya gsathya commented Jun 17, 2024

If a component uses the useRef hook directly then we type it's return value as a ref. But if it's wrapped in a custom hook then we lose out on this type information as the compiler doesn't look at the hook definition. This has resulted in some false positives in our analysis like the ones reported in #29160 and #29196.

This PR will treat objects named as ref or if their names end with the substring Ref, and contain a property named current, as React refs.

const ref = useMyRef();
const myRef = useMyRef2();
useEffect(() => {
  ref.current = ...;
  myRef.current = ...;
})

In the above example, ref and myRef will be treated as React refs.

Copy link

vercel bot commented Jun 17, 2024

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

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 18, 2024 10:14am

Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

Nice! Thoughts on checking that other properties (besides .current) are not accessed? If they are it's a good indication that the value wasn't actually a ref.

If a component uses the `useRef` hook directly then we type it's return
value as a ref. But if it's wrapped in a custom hook then we lose out
on this type information as the compiler doesn't look at the hook
definition. This has resulted in some false positives in our analysis
like the ones reported in facebook#29160 and facebook#29196.

This PR will treat objects named as `ref` or if their names end with the
substring `Ref`, and contain a property named `current`, as React refs.

```
const ref = useMyRef();
const myRef = useMyRef2();
useEffect(() => {
  ref.current = ...;
  myRef.current = ...;
})
```
In the above example, `ref` and `myRef` will be treated as React refs.
@gsathya
Copy link
Contributor Author

gsathya commented Jun 18, 2024

Nice! Thoughts on checking that other properties (besides .current) are not accessed? If they are it's a good indication that the value wasn't actually a ref.

There's still going to be false positives in either case, I'm not sure if this worth the extra complexity? Lets land this and try it on our codebase to see if there are false positives?

@react-sizebot
Copy link

Comparing: 92219ff...4bb18bd

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB +0.05% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.93 kB 497.93 kB = 89.26 kB 89.26 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.75 kB 502.75 kB = 89.96 kB 89.96 kB
facebook-www/ReactDOM-prod.classic.js = 597.17 kB 597.17 kB = 105.33 kB 105.33 kB
facebook-www/ReactDOM-prod.modern.js = 571.52 kB 571.52 kB = 101.27 kB 101.27 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Generated by 🚫 dangerJS against 4bb18bd

Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

That's fair, lets try it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants