-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[TextareaAutosize] Fix crash when used with React 18 & Suspense #33238
Conversation
I tried to create a test to reproduce the error but can't since the tests are running React 17 and the error only happens in React 18 |
I can confirm that the change fixes the issue. https://codesandbox.io/s/mui-issue-latest-forked-qjkc93?file=/src/index.tsx |
Maybe we can add a test later once we upgrade react to 18.x cc @mui/core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks a lot for your first contribution!
I think we should instead ask why |
I was trying to dig a bit deeper into why we are getting to this state, and it basically comes from the This is one simple reproduction - https://codesandbox.io/s/damp-microservice-ehjr7f?file=/src/app.js If you click on the Running ResizeObserver's function
{current: null}
Cleaning up ResizeObserver The If we are using traditional hide/show we are never getting to the stage where the ref is null - https://codesandbox.io/s/epic-tesla-wd1dr8?file=/src/_app.js Based on what I saw, the conclusion is that we should check for the refs being defined in all functions we use together with the |
On second thought this may be related to facebook/react#24594 I already have a project setup with the build from that PR will check if I can reproduce the issue there |
Will this be replaced by #33253? |
facebook/react#24594 doesn't seem to solve the problem, at least with the built I had from that branch. We can go ahead and merge this one and keep an eye for similar issues, basically using |
I really think you should wrap the call of That way we could check if we may be reading |
@@ -145,7 +145,6 @@ const TextareaAutosize = React.forwardRef(function TextareaAutosize(props, ref) | |||
// In React 18, state updates in a ResizeObserver's callback are happening after the paint which causes flickering | |||
// when doing some visual updates in it. Using flushSync ensures that the dom will be painted after the states updates happen | |||
// Related issue - https://github.com/facebook/react/issues/24331 | |||
// TODO: Do this only in the resize observer? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from #33253
Agree, more over we should only wrap the one call coming from the Codesandbox with the latest build: https://codesandbox.io/s/nice-river-sjn7kw?file=/package.json |
Looks like the fix for this was released? We're still seeing it in the latest (5.8.6 as of this writing) TypeError |
This is pretty annoying seeing as we don't even use the component but it still breaks stuff during development |
Please open a new issue and provide more details. |
Test case added in #38728. |
Fixes #32640