-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
[ShallowRenderer] Queue/rerender on dispatched action after render component with hooks #14802
[ShallowRenderer] Queue/rerender on dispatched action after render component with hooks #14802
Conversation
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.
LGTM. Thanks for fixing! :)
@@ -384,7 +384,7 @@ class ReactShallowRenderer { | |||
'an infinite loop.', | |||
); | |||
|
|||
if (componentIdentity === this._currentlyRenderingComponent) { | |||
if (componentIdentity === this._previousComponentIdentity) { |
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 change correct? It disagrees with what the comment inside says ("This is a render phase update"). What was it supposed to mean? Why was it changed?
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.
Yeah, I missed updating the comment =/
Before this change _dispatchAction
ignored calls after the render returned, this checks if its the same component and queue the action.
I reverted in #14839 — I appreciate the PR but I wanted to cut a patch soon and it's not obvious to me that this change is correct. (Although you're right the existing code is not either.) This code was originally written with no outside state updates in mind. It was written as 1:1 to the server renderer. It might need a larger refactoring to work with updates at arbitrary points in time. At the least, we should rename the variables and rewrite the comments that indicate these are only render-phase updates. With this change the logic doesn't match the comments and variables, and I'm worried this will make the code so confusing that nobody will be able to fix further problems in it. |
(I'd be happy if you could take this work. Let's just ensure the code matches the intention, and that we examine it all instead of minimal tweaks to "make it work".) |
I filed #14840 to track this. |
No problem @gaearon, I'll move the discussion to the issue. |
Thanks! |
Thanks for digging into this deeper. I have a larger fix ready that should handle this case properly. |
…mponent with hooks (#14802) * [shallow-renderer] Rerender on dispatched action out of render
…ender component with hooks (#14802)" (#14839) This reverts commit 6d4038f0a638d82e9e528f02cc5a86afb410cf11.
This should help make shallow render a viable option for testing components that use hooks.