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

[Shallow] Implement setState for Hooks and remount on type change #15120

Merged
merged 5 commits into from
Mar 15, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 15, 2019

Fixes #14840.

Alternative to #14841. While reviewing it, I realized some of the complexity in how we implemented it is relates to an existing bug in the shallow renderer. In particularly, rendering twice with different element type kept reusing the old class instance. I fixed this in 50f2262. (Now it behaves consistently on unmount/mount and on rendering a different element type.) While this has been broken for a long while, the old behavior of ignoring the type change plainly doesn't make sense — so if it breaks any tests, it means those tests were testing the wrong thing.

This allowed me to remove a bunch of fields and simplify the logic to match shallow renderer assumptions. In the last commit (4972adb), I'm adding support for state updates to shallow renderer. I think it's a bit simpler than what we tried in #14841.

I checked these changes don't break Enzyme test suite.

This worked in function components but was broken for classes. It incorrectly retained the old instance even if the type was different.
We only needed this because we didn't correctly reset based on type. Now we do so this can go away.
There was no particular reason it was set to element.type. We just wanted to check if something is a render phase update.
Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much simpler than my approach. Great stuff :)

@gaearon gaearon merged commit 8d60bd4 into facebook:master Mar 15, 2019
@gaearon gaearon deleted the so-shallow branch March 15, 2019 22:31
gaearon added a commit to gaearon/react that referenced this pull request Mar 22, 2019
…cebook#15120)

* Throw away old shallow renderer state on type change

This worked in function components but was broken for classes. It incorrectly retained the old instance even if the type was different.

* Remove _previousComponentIdentity

We only needed this because we didn't correctly reset based on type. Now we do so this can go away.

* Use _reset when unmounting

* Use arbitrary componentIdentity

There was no particular reason it was set to element.type. We just wanted to check if something is a render phase update.

* Support Hook state updates in shallow renderer
@gaearon gaearon mentioned this pull request Mar 22, 2019
@pgangwani
Copy link

pgangwani commented Mar 26, 2019

@gaearon does this include all hooks compatibility for Shallow Render. Can we take latest version to retest failed testcases in enzymejs/enzyme#2041 ? If I understood wrong, please suggest plan or reference if any

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 26, 2019

Feel free to test it. If you see an issue please file it.

This was referenced Oct 26, 2019
NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
…5120)

* Throw away old shallow renderer state on type change

This worked in function components but was broken for classes. It incorrectly retained the old instance even if the type was different.

* Remove _previousComponentIdentity

We only needed this because we didn't correctly reset based on type. Now we do so this can go away.

* Use _reset when unmounting

* Use arbitrary componentIdentity

There was no particular reason it was set to element.type. We just wanted to check if something is a render phase update.

* Support Hook state updates in shallow renderer
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.

Shallow renderer doesn't update on setState() with Hooks
4 participants