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

[Fix] `shallow`: `x.find()` should not change state of `x` #2061

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@sstern6
Copy link
Contributor

sstern6 commented Mar 19, 2019

Fixes #1916.

sstern6 added some commits Jan 15, 2019

[fix] `shallow`: `getNodesInternal`: delegate to the adapter’s `shoul…
…dUpdateComponent` method before updating the wrapper.

Fixes #1916.

@ljharb ljharb changed the title Issue1916 [Fix] `shallow`: `x.find()` should not change state of `x` Mar 20, 2019

@ljharb ljharb force-pushed the sstern6:issue1916 branch from d166048 to 531e6fb Apr 6, 2019

@ljharb
Copy link
Member

ljharb left a comment

I've rebased this.

I suspect we'll need to add shouldComponentUpdate to all the adapters.

@@ -1785,6 +1785,7 @@ describe('shallow', () => {
const wrapper = shallow(<MyComponent />, { disableLifecycleMethods: true });
expect(wrapper.find(Table).length).to.equal(0);
wrapper.instance().componentDidMount();
wrapper.update();

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 6, 2019

Member

the need for this addition concerns me; perhaps that's OK because most people won't be manually calling a lifecycle method, but this makes it seem like it might be a breaking change :-/

This comment has been minimized.

Copy link
@plemasantos

plemasantos Apr 9, 2019

@ljharb any idea or prediction on when this could be included in an official release? 👍

This comment has been minimized.

Copy link
@sstern6

sstern6 Apr 9, 2019

Author Contributor

finishing this up today

This comment has been minimized.

Copy link
@sstern6

sstern6 Apr 10, 2019

Author Contributor

i dont think we need this if we return true per your comment here: #2061 (comment). Checking though

@sstern6

This comment has been minimized.

Copy link
Contributor Author

sstern6 commented Apr 10, 2019

re > I suspect we'll need to add shouldComponentUpdate to all the adapters.

If we have the conditional to check if the shouldUpdateComponent is present why would we need to include it in all adapters?

Edit: some tests are failing, going to look into that as well, pushed some changes per your comments.

@@ -1785,6 +1785,7 @@ describe('shallow', () => {
const wrapper = shallow(<MyComponent />, { disableLifecycleMethods: true });
expect(wrapper.find(Table).length).to.equal(0);
wrapper.instance().componentDidMount();

This comment has been minimized.

Copy link
@sstern6

sstern6 Apr 10, 2019

Author Contributor

@ljharb was able to remove the wrapper.update() by returning true in the adapter function.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 23, 2019

Member

This still seems to be failing.

if (this[ROOT][WRAPPING_COMPONENT]) {
this[ROOT][WRAPPING_COMPONENT].unmount();
this[ROOT][WRAPPING_COMPONENT].update();

This comment has been minimized.

Copy link
@sstern6

sstern6 Apr 10, 2019

Author Contributor

Curious why these were added?

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 10, 2019

Member

to make tests pass; they were failing.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 10, 2019

why would we need to include it in all adapters?

@sstern6 because the issue this fixes should be fixed for all versions of react, not just react 16.

@ljharb ljharb force-pushed the airbnb:master branch 3 times, most recently from 40cc703 to 0a17404 Apr 12, 2019

@ljharb ljharb force-pushed the sstern6:issue1916 branch from d6ab780 to 4507396 Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.