Fixes bug when initial mount of a host component is hidden
#12294
Conversation
| // as the newProps. The updatePayload will contain the real change in | ||
| // this case. | ||
| const oldProps = current !== null ? current.memoizedProps : newProps; | ||
| let oldProps = current !== null ? current.memoizedProps : newProps; |
There was a problem hiding this comment.
Oops, forgot to delete :D
acdlite
force-pushed
the
fix-hidden-bug
branch
2 times, most recently
from
819b093 to
7ca44c9
Compare
| // we don't do the bailout and we have to reuse existing props instead. | ||
| } else if (memoizedProps === nextProps) { | ||
| return bailoutOnAlreadyFinishedWork(current, workInProgress); | ||
| if ( |
There was a problem hiding this comment.
renderExpirationTime === Offscreen?
There was a problem hiding this comment.
There's a test that fails when I do that, for some reason, haven't figured out why yet. Seems like it should do the same thing, since it will end up bailing out a few lines below, anyway.
| workInProgress.mode & AsyncMode && | ||
| shouldDeprioritizeSubtree(type, nextProps) | ||
| ) { | ||
| // Don't bail-out if this is hidden |
There was a problem hiding this comment.
As a general style guide we should invert this logic and remove the dead block because it is not always possible for the compiler to invert these conditionals and strip the extra code. Due to things like !(NaN > NaN) isn't the same as NaN <= NaN
There was a problem hiding this comment.
I mean it is always possible to wrap in !() but those three extra bytes can't be reclaimed.
acdlite
force-pushed
the
fix-hidden-bug
branch
from
7ca44c9 to
1a039f2
Compare
| workInProgress.mode & AsyncMode && | ||
| shouldDeprioritizeSubtree(type, nextProps) | ||
| ) { | ||
| if (renderExpirationTime === Never) { |
There was a problem hiding this comment.
This condition should be first because it is the fastest one. No property access.
There was a problem hiding this comment.
Ha, I was trying to avoid assigning the result of shouldDeprioritizeSubtree to a boolean :D
acdlite
force-pushed
the
fix-hidden-bug
branch
2 times, most recently
from
607100f to
ed4c171
Compare
`oldProps` was null. This went uncaught by the unit tests because ReactNoop did not use `oldProps` in either `prepareUpdate` or `completeUpdate`. I added some invariants so we don't regress in the future.
acdlite
force-pushed
the
fix-hidden-bug
branch
from
ed4c171 to
122a187
Compare
| } | ||
| if (newProps === null) { | ||
| throw new Error('Should have old props'); | ||
| } |
There was a problem hiding this comment.
Should be
if (oldProps === null || newProps === null) {
throw new Error('Should have old props');
}
?
5 participants
oldPropswas null. This went uncaught by the unit tests because ReactNoop did not useoldPropsin eitherprepareUpdateorcompleteUpdate. I added some invariants so we don't regress in the future.