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

[Masonry] Check if container or child exists to prevent error #29452

Merged
merged 10 commits into from Nov 9, 2021

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Nov 1, 2021

Closes #29362
Closes #29446

Problem:

  • Uncaught TypeError: Cannot read properties of null (reading 'clientWidth') at ResizeObserver.handleResize
  • code sandbox

Solution:

  • Make the following change at the start of handleResize() in order to prevent Cannot read properties of null error.
diff --git a/packages/mui-lab/src/Masonry/Masonry.js b/packages/mui-lab/src/Masonry/Masonry.js
index 73bc1e166d..e127668cd7 100644
--- a/packages/mui-lab/src/Masonry/Masonry.js
+++ b/packages/mui-lab/src/Masonry/Masonry.js
@@ -184,6 +184,9 @@ const Masonry = React.forwardRef(function Masonry(inProps, ref) {
 
   React.useEffect(() => {
     const handleResize = () => {
+      if (!masonryRef.current || !masonryRef.current.firstChild) {
+        return;
+      }
       const parentWidth = masonryRef.current.clientWidth;
       const childWidth = masonryRef.current.firstChild.clientWidth;
       const firstChildComputedStyle = window.getComputedStyle(masonryRef.current.firstChild);

@hbjORbj hbjORbj added bug 🐛 Something doesn't work component: masonry This is the name of the generic UI component, not the React module! labels Nov 1, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 1, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 70f5d59

@@ -0,0 +1,6 @@
import * as React from 'react';
Copy link
Member

@siriwatknp siriwatknp Nov 2, 2021

Choose a reason for hiding this comment

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

I think it is cheaper to do unit test. You can do something like

it('should not throw if children are empty', () => {
  expect(() => render(<Masonry columns={3} spacing={1} />)).not.to.throw()
})

Copy link
Member Author

Choose a reason for hiding this comment

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

Always love your code review, Jun.

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.

👍 Nicee

packages/mui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved
packages/mui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved
@hbjORbj hbjORbj requested a review from mnajdova November 3, 2021 09:36
@hbjORbj
Copy link
Member Author

hbjORbj commented Nov 3, 2021

Code sandboxes proving that this PR fixes
(1) the first issue: Uncaught Type Error is thrown on renders

(2) the second issue: Uncaught Type Error is thrown when switching between pages

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.

Check if the changes need to be applied in the other components that use the ResizeObserver.

packages/mui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved
packages/mui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved
packages/mui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved
@rgcl
Copy link

rgcl commented Nov 9, 2021

Please approve this 🥺

@hbjORbj hbjORbj merged commit f7594b8 into mui:master Nov 9, 2021
@brusacco
Copy link

brusacco commented Nov 14, 2021

I'm still having this error in NextJS using the Link component, when I use just a simple ahref there are no problems.

Unhandled Runtime Error
TypeError: Cannot read properties of null (reading 'clientWidth')

Call Stack
ResizeObserver.handleResize
node_modules/@mui/lab/Masonry/Masonry.js (171:0)

@hbjORbj
Copy link
Member Author

hbjORbj commented Nov 15, 2021

@brusacco Thanks for the report. Can you provide a codesandbox please? You can fork this template: https://codesandbox.io/s/mui-issue-latest-s2dsx

@netlifeapp
Copy link

Still happening on NextJS, i solved it by adding skeleton component:
<Masonry>{some.state?(masonryitems):(<Skeleton />)}</Masonry>

@hbjORbj
Copy link
Member Author

hbjORbj commented Dec 7, 2021

@netlifeapp

It doesn't look like this bug report has enough info for one of us to reproduce it.
Please provide a CodeSandbox (https://material-ui.com/r/issue-template-latest), a link to a repository on GitHub, or provide a minimal code example that reproduces the problem.
Here are some tips for providing a minimal example: https://stackoverflow.com/help/mcve

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: masonry This is the name of the generic UI component, not the React module!
Projects
None yet
7 participants