-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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 bailout broken in lazy components due to default props resolving #18491
Conversation
We should never compare unresolved props with resolved props. Since comparing resolved props by reference doesn't make sense, we use unresolved props in that case. Otherwise, resolved props are used.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 92c15f2:
|
@jddxf you probably may want to change the way you pull updated changes form react original repo to your fork because when you pull them out you leave a lot of references in all the other pull requests!!! |
oldState !== current.memoizedState | ||
) { | ||
workInProgress.effectTag |= Snapshot; | ||
} | ||
} | ||
instance.props = workInProgress.memoizedProps = newProps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, React would warn.
react/packages/react-reconciler/src/ReactFiberBeginWork.js
Lines 859 to 868 in 1fd4543
if (inst.props !== nextProps) { | |
if (!didWarnAboutReassigningProps) { | |
console.error( | |
'It looks like %s is reassigning its own `this.props` while rendering. ' + | |
'This is not supported and can lead to confusing bugs.', | |
getComponentName(workInProgress.type) || 'a component', | |
); | |
} | |
didWarnAboutReassigningProps = true; | |
} |
This also mirrors the logic at the end of the same function.
react/packages/react-reconciler/src/ReactFiberClassComponent.js
Lines 1139 to 1151 in 1fd4543
// If shouldComponentUpdate returned false, we should still update the | |
// memoized props/state to indicate that this work can be reused. | |
workInProgress.memoizedProps = newProps; | |
workInProgress.memoizedState = newState; | |
} | |
// Update the existing instance's state, props, and context pointers even | |
// if shouldComponentUpdate returns false. | |
instance.props = newProps; | |
instance.state = newState; | |
instance.context = nextContext; | |
return shouldUpdate; |
This was not needed before because it was already guaranteed by the condition in the branch.
oldProps === newProps && |
@bl00mber Sorry for that. I did force pushed the right commits but that didn't work. I had to delete the fork and it seems ok now. |
@@ -997,11 +997,13 @@ function updateClassInstance( | |||
|
|||
cloneUpdateQueue(current, workInProgress); | |||
|
|||
const oldProps = workInProgress.memoizedProps; | |||
instance.props = | |||
const unresolvedOldProps = workInProgress.memoizedProps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This says they're unresolved. But later we do this:
instance.props = workInProgress.memoizedProps = newProps;
So next time we would have resolved props in memoizedProps
? I don't quite follow the logic.
Is there a way we could make it consistent? So that memoizedProps
would always be unresolved, for example. Would that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we reset workInProgress.memoizedProps
to workInProgress.pendingProps
right after beginWork, workInProgress.memoizedProps
is always unresolved at the start of beginWork.
unitOfWork.memoizedProps = unitOfWork.pendingProps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then can we assign instance.props
only? Is there a reason for workInPorgress.memoizedProps
assignment? Sorry I'm being dense, it's been a while since I looked a tthis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. But unfortunately we have to keep their values in sync. Here is the reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think it makes sense to assign either of them.
In the sCU branch, instance.props
reassignment makes sense because the passed props actually did change. We want the most recent values inside of them to be accessible to the lifecycle methods, even if sCU "lies" about them not having changed.
In this branch, we've verified props are ===
. So the only reason they "changed" is because we resolved default props. That's not a compelling reason to give our components a different props object.
As you noted, memoizedProps
gets reset anyway. So it's confusing to assign it here if we know that's gonna happen.
I think we should fix whatever causes the warning instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change the warning condition to if (shouldUpdate && inst.props !== nextProps) {
then the warning won't fire. It will also make sense because the render wasn't called — so the warning message would have been misleading anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instance.props = workInProgress.memoizedProps = newProps;
This only gives our components a different props object when we've actually resolved default props. But in that case, components are already given a new props object.
react/packages/react-reconciler/src/ReactFiberClassComponent.js
Lines 1001 to 1004 in 2dddd1e
instance.props = | |
workInProgress.type === workInProgress.elementType | |
? oldProps | |
: resolveDefaultProps(workInProgress.type, oldProps); |
I think it's ok to remove this assignment if keeping
workInProgress.memoizedProps
and instance.props
stay the same until beginWork finishes is not a requirement. And that seems true now. So let's change the warning condition instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the new assignment in the bailout case and change the warning condition instead to only fire when shouldUpdate
is true
.
I cannot update this PR now due to refork. Closed in favour of #18539. |
Fix #17151
We should never compare unresolved props with resolved props. Since comparing resolved props by reference doesn't make sense, we use unresolved props in that case. Otherwise, resolved props are used.