Ignore text nodes in childrenOfInstInternal #604
Conversation
| it('should not attempt to get an instance for text nodes', () => { | ||
| const wrapper = mount(<div>B<span />C</div>); | ||
| const children = wrapper.children(); | ||
| expect(children.length).to.equal(1); |
There was a problem hiding this comment.
i agree it shouldn't attempt to get an instance - but the div has 3 children, not 1.
There was a problem hiding this comment.
That's inconsistent though. Even before this change, mounting an element with a single text node child returns 0 children.
const wrapper = mount(<div>A</div>);
wrapper.children().length
// 0There was a problem hiding this comment.
hmm - that's confusing to me. However, if the current behavior is that text nodes are ignored, then your change is consistent with that - so, LGTM
There was a problem hiding this comment.
Yeah it didn't make a lot of sense to me either. React is being a little inconsistent here, I'm going to look into it more on that side of things.
There was a problem hiding this comment.
@aweary did you learn anything about the React side of things?
There was a problem hiding this comment.
@lencioni _renderedChildren is null if the only child is a string node. Otherwise they're represented as ReactDOMTextComponent instances. I'm still not sure if there's a good reason for this.
|
@aweary mind giving this a rebase when you have a moment? |
|
BTW, the lint error you see in CI here looks like it was fixed by c07e61f, so a rebase onto master should resolve that for you. |
aweary
force-pushed
the
fix-text-children
branch
from
a8a1399 to
f53a63e
Compare
|
I'm actually hesitant to merge this just yet, I'd like to see if it's reasonable for |
|
Any idea on ETA? |
|
@spicyj would you mind weighing in on this? Is this something we could address internally with React? I know we're using internal APIs that are not supported, but I'm curious what you think. |
|
Hoping to see this merged soon. As it is I'm having to merge this branch manually to make enzyme useable. |
|
The merge of master into fix-text-children has broken MountedTraversal.js because of the new means of traversing |
| @@ -121,6 +121,9 @@ export function childrenOfInstInternal(inst) { | |||
| if (REACT013 && !node.getPublicInstance) { | |||
| return false; | |||
| } | |||
| if (typeof renderedChildren[key]._stringText !== 'undefined') { | |||
There was a problem hiding this comment.
The master branch is no longer iterating over the keys, and thus key is undefined here. Should be if (typeof node._stringText !== 'undefined') {
|
Alright, I'm going to fix the conflicts and then I think we should just merge. Even if we were to get React to fix this behavior moving forward, we still have to support versions with the current behavior. This solution isn't ideal, but I think it's better to consistent with how we treat text nodes, even if React isn't. |
Since we no longer iterate over the keys
aweary
force-pushed
the
fix-text-children
branch
from
b3524e2 to
8fa8717
Compare
|
@aweary: Incredible timing. Been stalking this PR for months. Gave up yesterday and published a fork to npm. Any idea when 2.7.1 gets released? |
Resolves #603
Currently React will return
nullfor_renderedChildrenif the only child is a text node. But if there is an actual DOM node as well, it will return that along with any sibling text nodes.This change ignores text nodes when calling
wrapper.childen()withmount. Withshallow, no instances are actually needed when parsing children, so this works already.So there's some inconsistency there. I think it makes more sense to let shallow return text nodes, but we could ignore them if we want consistency.
cc @ljharb