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

Jump to bottom

Relax validateDOMNesting warning for whitespace in table #6995

Closed
sophiebits opened this issue Jun 8, 2016 · 13 comments
Closed

Relax validateDOMNesting warning for whitespace in table #6995

sophiebits opened this issue Jun 8, 2016 · 13 comments
Labels
Difficulty: starter

Comments

@sophiebits
Copy link
Collaborator

See #5071 (comment) -- since we stopped wrapping text in spans, we should loosen the validation in validateDOMNesting a little. I think it should probably be fine to have whitespace anywhere in the tree?
@syranide
Copy link
Contributor

syranide commented Jun 8, 2016 •
edited
Loading

I think it should probably be fine to have whitespace anywhere in the tree?

Yah. I'm torn, considering we mostly validate for invalid nesting and not to be strict it seems fine to just let it through even though it's useless. However, non-whitespace strings are not allowed there and will break SSR.

@jimfb
Copy link
Contributor

jimfb commented Jun 8, 2016

@syranide I don't understand, why are you torn? This issue is only about whitespace, which is always safe, even with SSR, right?

@syranide
Copy link
Contributor

syranide commented Jun 8, 2016

@jimfb Yeah, it's safe, but it's also useless and there's virtually no reason for it to exist there. So warning could be a good thing too. It isn't really a big deal though, this is very much an edge-case, but still.

@mnpenner
Copy link
Contributor

mnpenner commented Jun 9, 2016

Thank for opening this ticket for/because of me, but I think the warning is not too bad actually. What if someone tries writing something like:

<tr>{someString}</tr>

Now the warning would be suppressed if someString is an empty/whitespace string, but it will become a legitimate problem as soon as a value is put in there, causing an error only some of the time, which is the worst kind of bug.

However, empty strings are falsey so avoiding the warning in situations such as I had,

<tr>{someString && <td/>}</tr>

Would actually be beneficial. But it's also not too hard for me to amend that code to make sure someString comes out a bool, so I'm leaning towards keeping the warning in.

Also, if it's going to create these <!-- react-text -->s, I'd rather avoid that, even if they're invisible.

@agiron123
Copy link
Contributor

I'd be interested in taking a stab at fixing this. I'll have a PR up later this week.

@joshhunt
Copy link
Contributor

joshhunt commented Jun 12, 2016 •
edited
Loading

For what it's worth, we saw this error when we had a comment at the end of a line in the <head>. For example, this produces an error:

return (
  <head>
    <title>React.js</title> {/* title tag! */}
  </head>
)

The space between the title tag and the JS expression comment throws the error. Easily fixed, but it was a bit surprising.

@syranide
Copy link
Contributor

@joshhunt Ah, that's part of the JSX parsing rules. If you replace your comment (which is actually an empty expression) with a string it makes a lot more sense, same rules apply.

@joshhunt
Copy link
Contributor

Yeah I got that - I figured an extra space char wound up in the generated response, but I figured there's nothing wrong with white space in the head tag and would fall under this issue?

@hkal
Copy link
Contributor

hkal commented Jun 20, 2016

Going to give this a go.

@hkal hkal mentioned this issue Jun 20, 2016
Closed
@wallacyyy
Copy link

Is this still active ? I can give a try if not :)

@aweary
Copy link
Contributor

aweary commented Oct 4, 2016

@wallacyyy there's an open PR for this at #7081

@sophiebits
Copy link
Collaborator Author

Fixed in #7515.

@wallacyyy
Copy link

@aweary I know, just asked because the last interaction was 2 months ago, so maybe the PR could not been active anymore :) Thx for the reply !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Difficulty: starter
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

No whitespace warning hkal/react
9 participants
@sophiebits @joshhunt @wallacyyy @agiron123 @mnpenner @syranide @hkal @aweary @jimfb