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: .parents: ensure that one .find call does not affect another #1781

Merged
merged 5 commits into from
Sep 21, 2018

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Aug 21, 2018

Fixes #1780.

These tests are failing; anyone who can help by posting a link to their branch would be very appreciated.

expect(bChildParents).to.have.lengthOf(1);
*/

const aChildParents = aChild.parents('.b');
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ljharb I believe this should this be const aChildParents = aChild.parents('.a');? (which fixes this test)

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, i'll fix that

@ljharb ljharb force-pushed the fix_find_statefulness branch from c294dc0 to aa74929 Compare September 6, 2018 06:32
@jgzuke
Copy link
Collaborator

jgzuke commented Sep 6, 2018

It looks like this issue is coming from the changes in #1499, the state is changing when getNodeInternal calls update. If we remove the update calls from getNode(s)Internal these tests pass.

The test can be reduced to

it.only('works when calling parents after getNodesInternal', () => {
  class Test extends React.Component {
    render() {
      return (
        <div>
          <div className="a">
            <div>A child</div>
          </div>
        </div>
      );
    }
  }

  const wrapper = shallow(<Test />);

  const aChild = wrapper.find({ children: 'A child' });
  expect(aChild.debug()).to.equal(`<div>
A child
</div>`);
  expect(aChild).to.have.lengthOf(1);

  // Fails when uncommented
  // wrapper.update();

  const aChildParents = aChild.parents('.a');
  expect(aChildParents.debug(`<div className="a">
<div>A child</div>
</div>`));
  expect(aChildParents).to.have.lengthOf(1);
});

@jgzuke
Copy link
Collaborator

jgzuke commented Sep 6, 2018

@ljharb when update gets called we get a new (but identical) root tree. The problem is when we call parents and go to get the pathToNode from our node to its root, the this[ROOT] references a different tree than this[NODES] (this[NODES] references the old tree) so we never find a match and always return null from pathToNode.

To illustrate

class Test extends React.Component {
  render() {
    return (
      <div>
        <div className="a">
          <div>A child</div>
        </div>
      </div>
    );
  }
}

const wrapper = shallow(<Test />);

const aChild = wrapper.find({ children: 'A child' });

const oldNode = aChild[NODES];
const oldRoot = aChild.root()[NODES];

wrapper.update();

const newNode = aChild[NODES];
const newRoot = aChild.root()[NODES];

console.log(oldNode === newNode); // true
console.log(oldRoot === newRoot); // false

Im not really sure what the correct fix is here. Im also not sure why we are updating on all getNode(s)Internal calls. But it seems like given that the a nodes parent can always be updated, we need some way to keep our non root [NODES] references up to date?

Im also not sure whether this affects anything else besides the pathToNode, it seems like any time we save a reference to the non root node this could come up

@ljharb ljharb force-pushed the fix_find_statefulness branch from aa74929 to d31b829 Compare September 6, 2018 20:12
@ljharb
Copy link
Member Author

ljharb commented Sep 6, 2018

I think you're exactly right, and it's the reason I had to do the weird hierarchy stuff for simulateError's componentStack.

I think that wrapper.update() should update the non-root NODES list as well, such that === holds.

@jgzuke
Copy link
Collaborator

jgzuke commented Sep 7, 2018

@ljharb 👍 I will try to make that change

@blainekasten blainekasten removed their request for review September 7, 2018 12:53
@ljharb ljharb force-pushed the fix_find_statefulness branch from d31b829 to e792c97 Compare September 11, 2018 22:56
@ljharb ljharb force-pushed the fix_find_statefulness branch from e792c97 to 4dd2780 Compare September 19, 2018 22:27
@ljharb ljharb merged commit 9c562b1 into master Sep 21, 2018
@ljharb ljharb deleted the fix_find_statefulness branch September 21, 2018 21:48
ljharb added a commit that referenced this pull request Oct 5, 2018
 - [new] `mount`: `.state()`/`.setState()`: allow calling on children (#1802)
 - [new] `configuration`: add `reset`
 - [fix] `makeOptions`: ensure that config-level `attachTo`/`hydrateIn` are inherited into wrapper options (#1836)
 - [fix] `shallow`/`Utils`: call into adapter’s `isCustomComponentElement` if present (#1832)
 - [fix] `shallow`/`mount`: throw an explicit error when state is null/undefined
 - [fix] freeze ROOT_NODES for child wrappers (#1811)
 - [fix] `shallow`: `.parents`: ensure that one `.find` call does not affect another (#1781)
 - [fix] `mount`: update after `simulateError` (#1812)
 - [refactor] `mount`/`shallow`: `getElement`: use `this.single`
 - [deps] update `babel-preset-airbnb`, `chai`, `eslint`, `mocha`, `enzyme-adapter-utils`, `react-is`, `airbnb-js-shims`
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.

2 participants