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

hasChildTextNode doesn’t handle numeric children #21

Open
ticky opened this issue Mar 30, 2015 · 4 comments
Open

hasChildTextNode doesn’t handle numeric children #21

ticky opened this issue Mar 30, 2015 · 4 comments

Comments

@ticky
Copy link

ticky commented Mar 30, 2015

The code at assertions.js#L34-L35 doesn’t handle numeric children and throws an uncaught error trying to access properties of the number object in these instances.

This is a seemingly common thing in the wild. Things like react-data-components emit elements whose only children are numbers.

I’m happy to provide a PR, but not sure what the best option is here, but it may be to handle numeric children as strings. That said, this perhaps brings up a question about how “implicit” string conversions should be handled.

@ticky
Copy link
Author

ticky commented Mar 30, 2015

Upon further reading, this seems to be handled (just not in the way I’d personally expect) by #10.

@kloots
Copy link
Collaborator

kloots commented May 13, 2015

Can you expand on what you expected? Further, not sure what value there would be in making assertions against numeric children.

@ticky
Copy link
Author

ticky commented May 14, 2015

More or less a matter of laziness. If you render a number in a react component, it’s generally displayed as text.

While I don’t consider myself an expert, and can’t be entirely certain there would be no negative effects effects, I’d suggest a good approach would be to update the tests in that file so we only check;

  1. if the child is null or undefined
  2. if the child is an image
  3. if the child is a React component and has children

Then if it hasn’t matched any of those, treat the child as a string. As far as I’m aware, anything which didn’t fit those checks would be treated as such by React, anyway.

Alternately, react-a11y could mandate that strings must be strings, requiring lots of toString() calls in existing code.

@kloots
Copy link
Collaborator

kloots commented May 21, 2015

Thanks for following up. Why don't you put together a PR representing the change you'd like to see so we can review it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants