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

Fix(Portal) check for rootNode rendering to prevent race condition #2457

Merged
merged 2 commits into from
Feb 2, 2018

Conversation

Brantron
Copy link
Contributor

We rely heavily on Semantic-UI-React and have had our monitoring reporting the following issue

TypeError: Cannot read property 'firstElementChild' of null

We've investigated it a few times but recently stumbled upon this issue #2401

This commit has been able to successfully prevent this issue without introducing any new issues.

@codecov-io
Copy link

codecov-io commented Jan 23, 2018

Codecov Report

Merging #2457 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2457      +/-   ##
==========================================
- Coverage   99.74%   99.74%   -0.01%     
==========================================
  Files         154      154              
  Lines        2699     2694       -5     
==========================================
- Hits         2692     2687       -5     
  Misses          7        7
Impacted Files Coverage Δ
src/addons/Portal/Portal.js 100% <100%> (ø) ⬆️
src/behaviors/Visibility/Visibility.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aed4f01...204b94a. Read the comment docs.

@Brantron Brantron changed the title check for rootNode when rendering Portals to prevent race condition Fix(Portal) check for rootNode rendering to prevent race condition Jan 24, 2018
@levithomason
Copy link
Member

levithomason commented Jan 25, 2018

Let's add a failing test that validates the fix. The fix location is also suspect. If this.rootNode were null or undefined at line 364, where the check is currently, then line 354 would throw a TypeError before ever reaching that point:

    this.rootNode.className = className || ''

@Brantron
Copy link
Contributor Author

@levithomason, excellent point. I dug a little further and came across facebook/react#9808 which highlights

The problem is unstable_renderSubtreeIntoContainer is async and can be called after componentDidUpdate. When refs are used in componentDidUpdate, they can be undefined due to async nature of unstable_renderSubtreeIntoContainer.

Thanks for the guidance. I've refactored a bit and added a test.

@Brantron
Copy link
Contributor Author

@levithomason ping

@levithomason
Copy link
Member

Ah, nice find! I've done some manual testing and this all looks great. Thanks for the detective work.

@levithomason levithomason merged commit 9e29d73 into Semantic-Org:master Feb 2, 2018
@aloey
Copy link

aloey commented Feb 2, 2018

Thank you so much for resolving this issue!
Any chance you will be updating the npm package soon?

@levithomason
Copy link
Member

Released in semantic-ui-react@v0.78.0.

@levithomason
Copy link
Member

@aloey I shoot for a release every weekend. Sometimes life prevents it.

@JsGvDev
Copy link

JsGvDev commented Apr 11, 2018

Thanks folks to fix this error. I've spent a few hours to find out about this bug, because I only could get this error from Portal but into 'node_modules/react/cjs/react.development.js', until I can isolate that error. Nothing into the app is broken, everything was working fine, but I got that error #2401 from console. I was using semantic-ui-react@v0.76.0, after updating semantic-ui-react@v0.78.0 that error is gone.

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

Successfully merging this pull request may close these issues.

5 participants