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

[Tabs] Fix null reference in ScrollbarSize after unmounting #36485

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

rkdrnf
Copy link
Contributor

@rkdrnf rkdrnf commented Mar 10, 2023

Closes #26587

As per https://reactjs.org/blog/2020/08/10/react-v17-rc.html#effect-cleanup-timing, timeout callback set by debounce can be called before effect cleanup, which can leads to calling setMeasurements with nodeRef.current unset.

@mui-bot
Copy link

mui-bot commented Mar 10, 2023

Netlify deploy preview

https://deploy-preview-36485--material-ui.netlify.app/

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 5ded5a2

@rkdrnf rkdrnf changed the title use local element reference instead of referencing node inside timeout [Tabs] use local element reference instead of referencing node inside timeout Mar 10, 2023
@rkdrnf rkdrnf changed the title [Tabs] use local element reference instead of referencing node inside timeout [Tabs] fix occasional null reference in Tabs ScrollbarSize element on demounting Mar 10, 2023
@rkdrnf rkdrnf changed the title [Tabs] fix occasional null reference in Tabs ScrollbarSize element on demounting [Tabs] fix occasional null reference in Tabs ScrollbarSize element on unmounting Mar 10, 2023
@zannager zannager added the component: tabs This is the name of the generic UI component, not the React module! label Mar 10, 2023
@zannager zannager requested a review from mnajdova March 10, 2023 08:23
const setMeasurements = () => {
scrollbarHeight.current = nodeRef.current.offsetHeight - nodeRef.current.clientHeight;
const setMeasurements = (element) => {
scrollbarHeight.current = element.offsetHeight - element.clientHeight;
};

React.useEffect(() => {
Copy link
Member

@oliviertassinari oliviertassinari Mar 10, 2023

Choose a reason for hiding this comment

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

Would it be a simpler fix with a one-line change? Like we did in #25619 since with layout effect, the cleanup runs synchronously.

Suggested change
React.useEffect(() => {
useEnhancedEffect(() => {

Copy link
Contributor Author

@rkdrnf rkdrnf Mar 14, 2023

Choose a reason for hiding this comment

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

@oliviertassinari Tried to avoid synchronous effect if it can be avoided in another way because it can impose unnecessary impact on performance. In this case, the effect would not be called frequently so it would be ok to do that in this feature I think. Thanks for the review.

Copy link
Member

@oliviertassinari oliviertassinari Mar 20, 2023

Choose a reason for hiding this comment

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

I feel that the synchronous layout effect makes more sense since we want to detect layout changes. It also simplifies a bit the logic, as it guarantees that the element is defined.

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Mar 10, 2023
@rkdrnf rkdrnf requested review from oliviertassinari and removed request for mnajdova March 14, 2023 03:21
@rkdrnf
Copy link
Contributor Author

rkdrnf commented Mar 14, 2023

@mnajdova Sorry I accidentally removed you from reviewers while requesting review again.

@oliviertassinari oliviertassinari added the package: material-ui Specific to @mui/material label Mar 20, 2023
@oliviertassinari oliviertassinari removed their request for review March 20, 2023 00:32
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 20, 2023

The logic makes sense to me, but it's no longer my role to review PRs. I have removed myself.

@oliviertassinari oliviertassinari changed the title [Tabs] fix occasional null reference in Tabs ScrollbarSize element on unmounting [Tabs] Fix null reference in ScrollbarSize after unmounting Mar 20, 2023
@rkdrnf
Copy link
Contributor Author

rkdrnf commented Apr 10, 2023

I hope it can get merged soon because it has a very minor change.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

It's a great first pull request on Material UI 👌 Thank you for working on it!

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: tabs This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tabs] Sometimes gives an error of "null is not an object" if it's scrollable
5 participants