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

[TextareaAutosize] Fix crash when used with React 18 & Suspense #33238

Merged
merged 4 commits into from Jun 24, 2022

Conversation

howlettt
Copy link
Contributor

Fixes #32640

@mui-bot
Copy link

mui-bot commented Jun 21, 2022

Details of bundle changes

Generated by 🚫 dangerJS against fe1acc0

@mnajdova mnajdova requested a review from siriwatknp June 21, 2022 09:56
@mnajdova mnajdova added bug 🐛 Something doesn't work component: TextareaAutosize The React component. labels Jun 21, 2022
@siriwatknp siriwatknp added the PR: needs test The pull request can't be merged label Jun 21, 2022
@howlettt
Copy link
Contributor Author

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

@siriwatknp
Copy link
Member

I can confirm that the change fixes the issue. https://codesandbox.io/s/mui-issue-latest-forked-qjkc93?file=/src/index.tsx

@siriwatknp
Copy link
Member

siriwatknp commented Jun 22, 2022

Maybe we can add a test later once we upgrade react to 18.x

cc @mui/core

Copy link
Member

@siriwatknp siriwatknp left a 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!

@eps1lon
Copy link
Member

eps1lon commented Jun 22, 2022

I think we should instead ask why syncHeight is called while input is null. This proposal might hide actual bugs in the implementation.

@mnajdova
Copy link
Member

I was trying to dig a bit deeper into why we are getting to this state, and it basically comes from the ResizeObserver's handle function being invoked when suspense's fallback is being rendered (as there is a change in the layout). What is strange is that the resize observer's handler is being invoked but the ref.current is null, which is a state I was thinking never should have happened. The problem comes from the fact that the cleanup function for the resize observer is being invoked when suspense is replaced with the lazily loaded component, which is a bit too late in this case.

This is one simple reproduction - https://codesandbox.io/s/damp-microservice-ehjr7f?file=/src/app.js

If you click on the Toggle view button you will notice the following sequence of console logs:

Running ResizeObserver's function 
{current: null}
Cleaning up ResizeObserver 

The {current: null} is the problem. In my opinion, here we should have seen only the "Cleaning up ResizeObserver " log.

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 ResizeObserver. @eps1lon do you think this is expected behavior? Looks like a bug in React 18 to me if I am being honest.

@mnajdova
Copy link
Member

mnajdova commented Jun 22, 2022

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

@eps1lon
Copy link
Member

eps1lon commented Jun 22, 2022

Will this be replaced by #33253?

@mnajdova
Copy link
Member

Will this be replaced by #33253?

#33253 does not solve this issue. I will update the description on that PR to better match the fix it is solving.


For the test we need to use Suspense if we want to reproduce the issue. The codesandbox on the issue is basically what should be the test case.

@mnajdova
Copy link
Member

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 Suspense together with a component using ResizeObserver that access ref.

@eps1lon
Copy link
Member

eps1lon commented Jun 23, 2022

I really think you should wrap the call of syncHeight instead of exiting early in syncHeight. The proposal might hide legitimate bugs.

That way we could check if we may be reading inputRef.current at the wrong time

@@ -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?
Copy link
Member

Choose a reason for hiding this comment

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

Leftover from #33253

@mnajdova
Copy link
Member

I really think you should wrap the call of syncHeight instead of exiting early in syncHeight. The proposal might hide legitimate bugs.

That we could check if we may be reading inputRef.current at the wrong time

Agree, more over we should only wrap the one call coming from the ResizeObserver's handler. I've updated the code and added comment with an explanation. We can add a test case after we migrate the codebase to React 18.

Codesandbox with the latest build: https://codesandbox.io/s/nice-river-sjn7kw?file=/package.json

@mnajdova mnajdova changed the title [TextareaAutosize] Fix error when input is null [TextareaAutosize] Fix crash when used with React 18 & Suspense Jun 24, 2022
@mnajdova mnajdova merged commit 215122c into mui:master Jun 24, 2022
@mnajdova mnajdova mentioned this pull request Jun 24, 2022
6 tasks
@dkadrios
Copy link

Looks like the fix for this was released? We're still seeing it in the latest (5.8.6 as of this writing)
😟

TypeError
Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.
Call Stack
 eval
  node_modules/@mui/material/node_modules/@mui/base/TextareaAutosize/TextareaAutosize.js:65:43
 ResizeObserver.eval
  node_modules/@mui/material/node_modules/@mui/base/TextareaAutosize/TextareaAutosize.js:128:7
 later
  node_modules/@mui/material/node_modules/@mui/utils/esm/debounce.js:12:12

@Zache
Copy link

Zache commented Sep 5, 2022

This is pretty annoying seeing as we don't even use the component but it still breaks stuff during development

@siriwatknp
Copy link
Member

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.

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
@oliviertassinari oliviertassinari removed the PR: needs test The pull request can't be merged label Sep 16, 2023
@oliviertassinari
Copy link
Member

Test case added in #38728.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: TextareaAutosize The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Input] Failed to execute 'getComputedStyle' on 'Window' when suspense triggers on react 18
8 participants