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

Add tests on broken .parent() use-case #769

Merged
merged 2 commits into from
Jul 6, 2018
Merged

Add tests on broken .parent() use-case #769

merged 2 commits into from
Jul 6, 2018

Conversation

just-boris
Copy link
Contributor

@just-boris just-boris commented Jan 12, 2017

When I traverse through React components in a mounted tree, it can't find parent and fails with the error

TypeError: Cannot read property 'reverse' of null
   at parentsOfInst (src/MountedTraversal.js:255:10)
   at ReactWrapper.<anonymous> (src/ReactWrapper.jsx:623:35)
   at ReactWrapper.single (src/ReactWrapper.jsx:916:21)

See added test as a reproduction example.

As far as I could track, it happens, because the node can't find itself because component wrapper the wrong children wrapper.children() in this test returns an empty array, however we have a node inside.

@ljharb
Copy link
Member

ljharb commented Jan 12, 2017

(linking to #410)

it('should work with components in the tree', () => {
const Foo = () => (<div className="bar" />);
const wrapper = mount(<Foo />);
expect(wrapper.find('.bar').parent()).to.equal(wrapper.find('Foo'));
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced this should actually work - Foo isn't supposed to be in the tree at all; only what Foo renders is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, not sure what should be at the right side. I haven't had a chance to look what it returns because it fails now.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a similar test to ShallowWrapper, so we can compare the behaviors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Shallow wrapper returns empty wrapper in this case.

Also, I have updated test for mounted wrapper. Now it expects that it has.root as a parent. It is not possible to do the same for shallow because it doesn't render components deeply.

@just-boris
Copy link
Contributor Author

Hello, @ljharb
I have updated the test and added a new one for shallow render.
Any more feedback?

},
});
const wrapper = shallow(<Foo />);
expect(wrapper.find('.bar').parent()).to.have.length(0);
Copy link
Member

Choose a reason for hiding this comment

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

In this case, .find('.bar') is the root - I'm not even sure .find('.bar') will find anything. Could you add an assertion that wrapper.find('.bar') has length 1?

Copy link
Contributor Author

@just-boris just-boris Jan 17, 2017

Choose a reason for hiding this comment

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

Done.
Shallow test is still passing, mounted is not.

@@ -257,7 +257,7 @@ export function pathToNode(node, root) {
}

export function parentsOfInst(inst, root) {
return pathToNode(inst, root).reverse();
return (pathToNode(inst, root) || []).reverse();
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems to me that pathToNode should just always return an array instead of having this || [] logic.

Copy link
Member

Choose a reason for hiding this comment

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

When I change that, another test breaks that I can't figure out. I'll push that commit up so you can play with it.

@alexreardon
Copy link

any reason this was abandoned?

@ljharb
Copy link
Member

ljharb commented Mar 21, 2017

@alexreardon it's not abandoned, it's just that nobody's provided a fix yet. I updated the PR just a few days ago

@alexreardon
Copy link

I missed that. Cheers

@dreadwail
Copy link

@alexreardon if you're looking for a quick (bad, slow) workaround:

const parent = ofNode => ofNode.root.findWhere(n => n.node == ofNode.node.parentNode);

warning: you probably shouldn't use this because its a pretty gross hack and quite slow

@zver-in
Copy link

zver-in commented Dec 25, 2017

any updates?

@ljharb
Copy link
Member

ljharb commented Dec 31, 2017

No updates; but I think this is the same as #1235

@ljharb
Copy link
Member

ljharb commented Jul 6, 2018

This appears to have been addressed in v3, as the tests are now passing. I'm going to merge the tests and close the linked issue.

@ljharb ljharb merged commit aa52bc2 into enzymejs:master Jul 6, 2018
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.

6 participants