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

Don't wrap functional components in react >=16.5 to shallow render #2014

Merged

Conversation

rodrigopr
Copy link
Contributor

@rodrigopr rodrigopr commented Feb 8, 2019

This is necessary because ReactShallowRender checks for equality in function components to decide if it should keep hooks state between renders.

This along with facebook/react#14802 / facebook/react#14840 should help in test components with some of the core hooks.
link #2011 , #2008 and #1996

@rodrigopr rodrigopr force-pushed the rodrigopr/cache-shallowed-wrapper-element branch from 454c51c to ae30cfd Compare February 8, 2019 21:12
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems great! Can you add a test that fails without this change?

@rodrigopr
Copy link
Contributor Author

rodrigopr commented Feb 11, 2019

@ljharb sure, I guess I'll wait for the react PR to be merged to do a more meaningful test for it.

@gaearon
Copy link
Contributor

gaearon commented Mar 22, 2019

I don't understand what this fix is supposed to do or why it makes sense. Could you please explain it?

@gaearon
Copy link
Contributor

gaearon commented Mar 22, 2019

Huh I see. I didn't realize Enzyme wraps the passed type... In that case this fix makes sense.
(Why does Enzyme do this?)

@rodrigopr rodrigopr force-pushed the rodrigopr/cache-shallowed-wrapper-element branch from b202023 to 2f6f002 Compare March 23, 2019 22:25
@rodrigopr
Copy link
Contributor Author

rodrigopr commented Mar 23, 2019

Seems like It was introduced to solve #1683, which is linked to issue facebook/react#13141 fixed since react 16.5.0.
On version 16.8.5 the tests passes without any wrapper.
What do you think @ljharb, @gaearon?

For now, I've updated the PR with tests

@gaearon
Copy link
Contributor

gaearon commented Mar 24, 2019

I'd remove the wrapper if we're targeting 16.5+.

@ljharb
Copy link
Member

ljharb commented Mar 24, 2019

That’s definitely something we can and should do - and it can be done in the adapter. We have all sorts of version-conditional hacks and bugfixes patches in the adapters.

@rodrigopr rodrigopr changed the title Cache wrapped function element in shallow renderer Don't wrap functional components in react >=16.5 to shallow render Mar 29, 2019
@rodrigopr
Copy link
Contributor Author

@ljharb I've updated the PR approach, to only wrap the component fn on version < 16.5

(...args) => Component(...args), // eslint-disable-line new-cap
Component,
);
cachedComponent = Component;
Copy link
Member

Choose a reason for hiding this comment

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

this line is important here; we don't want to create a new wrapper every time. Can we preserve it?

Copy link
Contributor Author

@rodrigopr rodrigopr Apr 1, 2019

Choose a reason for hiding this comment

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

PR updated with the requested change. I can also simplify the wrap-function using lodash.memoize or similar, if you think is worth adding the dep.

Also, a test started failing, I've tracked to this change:
abed5df (it started failing because it no longer tries to wrap the component)
I've reverted it since React.Memo also work on class component, but let me know if there is a better approach.

Copy link
Member

Choose a reason for hiding this comment

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

Nah, no need to add the dep.

Your change seems good :-)

@ljharb ljharb force-pushed the rodrigopr/cache-shallowed-wrapper-element branch from abed5df to f695b6a Compare April 1, 2019 07:46
@ljharb
Copy link
Member

ljharb commented Apr 1, 2019

k, i've rebased this on top of a major tests refactor; i'm not sure if your tests are still passing.

@rodrigopr
Copy link
Contributor Author

Thanks, seems the test still pass after the rebase

@ljharb ljharb merged commit f695b6a into enzymejs:master Apr 1, 2019
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.

3 participants