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.

Sign up for GitHub

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

Ignore text nodes in childrenOfInstInternal #604

Merged
merged 2 commits into from
Jan 10, 2017
Merged

Ignore text nodes in childrenOfInstInternal #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4dd0b4
Ignore text nodes in childrenOfInstInternal
Sep 22, 2016
8fa8717
Use node._stringText instead of renderedChildren
Jan 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/MountedTraversal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ export function childrenOfInstInternal(inst) {
if (REACT013 && !node.getPublicInstance) {
return false;
}
if (typeof node._stringText !== 'undefined') {
return false;
}
return true;
}).map((node) => {
if (!REACT013 && typeof node._currentElement.type === 'function') {
Expand Down
6 changes: 6 additions & 0 deletions test/ReactWrapper-spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1910,6 +1910,12 @@ describeWithDOM('mount', () => {
expect(children.at(1).hasClass('baz')).to.equal(true);
});

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);
Copy link
Member

@ljharb ljharb Sep 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

i agree it shouldn't attempt to get an instance - but the div has 3 children, not 1.

Copy link
Collaborator Author

@aweary aweary Sep 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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
      // 0

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aweary did you learn anything about the React side of things?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

});

describeIf(!REACT013, 'stateless function components', () => {
it('should handle mixed children with and without arrays', () => {
const Foo = props => (
Expand Down