fix(react-virtual): handle scrollElement cleanup after HMR
#820
Conversation
hophiphip
changed the title
scrollElement cleanup on HMRscrollElement cleanup after HMR
hophiphip
changed the title
scrollElement cleanup after HMRscrollElement cleanup after HMR
|
@piecyk can this be merged? I think this fixes the white space issue after HMR |
|
@tannerlinsley Hey, can you take a look at this PR? Can it be merged? |
packages/react-virtual/src/index.tsx
Outdated
| React.useEffect(() => { | ||
| if (!instance.scrollElement) { | ||
| instance._willUpdate() | ||
| } | ||
|
|
There was a problem hiding this comment.
I think better would be to change the useEffect to useIsomorphicLayoutEffect
| React.useEffect(() => { | |
| if (!instance.scrollElement) { | |
| instance._willUpdate() | |
| } | |
| useIsomorphicLayoutEffect(() => { |
then order will be correct
instance._didMount, cleanup
instance._willUpdate, cleanup
instance._didMount
instance._willUpdate
There was a problem hiding this comment.
Hmm, interesting, was there a reason useEffect was used instead of useIsomorphicLayoutEffect since the beginning?
There was a problem hiding this comment.
Overall when possible useEffect should preferred, there was no clear reason till now ot use useIsomorphicLayoutEffect imho.
useLayoutEffect is a version of useEffect that fires before the browser repaints the screen.
As this is called in limited way changing it to layout effect should be fine.
There was a problem hiding this comment.
@hophiphip please update the PR, then we can merge it! Thanks for looking into this.
There was a problem hiding this comment.
@piecyk, updated PR
hophiphip
force-pushed
the
react-hmr-fix
branch
from
9a6b7a4 to
5ab9a3b
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 5ab9a3b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
When HMR is triggered
useEffectcleanup function will call function returned frominstance._didMount().This will set
scrollElementto null andVirtualizerwill stop working.I added an additional check if
scrollElementis null and if it is - callinstance._willUpdate().This PR resolves #750.