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

DOM nesting warning is confusing for whitespace (and text in general) #5071

Closed
jimfb opened this issue Oct 7, 2015 · 20 comments
Closed

DOM nesting warning is confusing for whitespace (and text in general) #5071

jimfb opened this issue Oct 7, 2015 · 20 comments

Comments

@jimfb
Copy link
Contributor

jimfb commented Oct 7, 2015

When a user puts a textnode (whitespace) in an illegal location, our warning complains about a span element (because we wrap text - including whitespace - in a span). This is confusing for new users, since there is no span present in their code and the introduction of the span tag is an implementation detail of React. We should fix the warning to better align with the code the user is looking at.

var rows = <tr></tr>;
React.render(<table><tbody> {rows} </tbody></table>, document.getElementById('container'));
@loganhenson
Copy link

See #5098

Need to cover all cases...

  1. <span></span> in <table></table>
  2. <span></span> in <tbody></tbody>
  3. <span></span> in <thead></thead>
  4. <span></span> in <tfoot></tfoot>
  5. <span></span> in <tr></tr>

any others I am missing? See revised #5099

@jimfb
Copy link
Contributor Author

jimfb commented Oct 8, 2015

@spicyj I think this might be a bit advanced to be a good-first-bug. Specifically, we need to handle three general cases:

  • The user actually specified a span: <table><span>foo</span></table>. In which case, we should indicate that there is a span.
  • The user did not specify a span, but did specify child text: <table>foo</table>
  • The user did not specify a span, but did specify only whitespace: <table> </table>. In which case, we should call out the whitespace, because if we just call it text, it'll be easy to miss and be confused about what's going on. Whitespace includes tabs, newlines, and spaces.

@antsmartian
Copy link
Contributor

@jimfb Hmmm, looks like something is wrong with master branch (from your above comments and example). The code snippet

var rows = <tr></tr>;
React.render(<table><tbody> {rows} </tbody></table>, document.getElementById('container'));

with master prints the warning:

validateDOMNesting(...): <undefined> cannot appear as a child of <tr>. See tr > .

But it says undefined not span. However with 0.14 branch, I can see the warning with span. I was about to fix this issue, but got confused with undefined thing.

Apparently the code on validation part from 0.14 and master are different though:

(0.14 branch):

validateDOMNesting(
          'span',
          null,
          context[validateDOMNesting.ancestorInfoContextKey]
);

(master)

validateDOMNesting(this._tag, this, parentInfo);

In this case this._tag is undefined as we didn't wrap our text component in any tag. But doing so with say for example p tag, prints p instead of undefined

@antsmartian
Copy link
Contributor

@jimfb The mentioned three cases by you make sense. I had an rough idea for fixing this issue:

  1. First we need to identify the validateDOMNesting actually does validation on ReactDOMTextComponent. I thought I can check via Object.prototype.toString.call the type of incoming object to validateDOMNesting. Suggest this is the right way in the react community.
  2. If we could able to figure out the type if ReactDOMTextComponent, then we can redirect the check to say other method, where we can check these cases and appropriately print the warnings as desired.

Let me know if my thought process is correct or not.

@jimfb
Copy link
Contributor Author

jimfb commented Oct 16, 2015

I could take a guess on these, but I think it would be best if @spicyj could jump in here, as he wrote the validation code and knows it best.

@sophiebits
Copy link
Collaborator

Thanks, that's a bug, fixed in #5198.

@antsmartian
Copy link
Contributor

@spicyj Looks like you have just fixed the undefined part. Still this issue remains right?

Still the warning includes span in its text.

@antsmartian
Copy link
Contributor

@spicyj Ping.

@sophiebits
Copy link
Collaborator

Yes.

@antsmartian
Copy link
Contributor

@spicyj: I thought of doing the fix for this issue, so on this validations line :

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactDOMTextComponent.js#L96

I thought of changing this to roughly :

if(this._stringText.indexOf('\t') > -1 || this._stringText.indexOf('\n') >-1 || this._stringText.indexOf(' ') > -1 )
  validateDOMNesting("Whitespaces", this, parentInfo);
else
  validateDOMNesting("Text " + this._stringText, this, parentInfo);

I tested it manually in the browser and it works as expected. But the message is getting displayed as (even for text it adds < and >):

Warning: validateDOMNesting(...): <Whitespaces> cannot appear as a child of <tbody>. See tbody > Whitespaces.

if we do:

var rows = <tr>Hello world</tr>;
React.render(<table><tbody>{rows} </tbody></table>, document.getElementById('container'));

Kindly let me know if the approach taken over here is correct. So that I can go and give a PR.

@antsmartian
Copy link
Contributor

@spicyj Ping.

@sophiebits
Copy link
Collaborator

You can change the signature of validateDOMNesting if you have to. Also, your check for whitespace isn't right: it should only refer to whitespace if all of the text is whitespace, not if there's any whitespace character (which is what you have).

@troydemonbreun
Copy link
Contributor

Looks like this was fixed in #5753 ? Was thinking of picking it up, but upon investigation, it appears fixed and maybe this can be closed.

@sophiebits
Copy link
Collaborator

We don't explicitly call out whitespace but yeah, hopefully this is clear enough now.

@mnpenner
Copy link
Contributor

mnpenner commented Jun 8, 2016

I'm getting whitespace in here somehow:

<table className="billing-table">
 <tbody>{data.billing_type && <tr>
         {data.billing_type === 'HOURLY'
         && <td className="name"><strong>Base:</strong> {kb.formatCurrency(data.hourly_rate)}/hr &times; {kb.formatHours(data.hours)}</td>}
         {data.billing_type === 'FLAT'
         && <td className="name"><strong>Flat Rate</strong></td>}
         <td className="amount">{kb.formatCurrency(baseAmount)}</td>
     </tr>}{feeList.map(fee => {
         return <tr key={fee.key}>
             <td className="name">{fee.name}</td>
             <td className="amount">{fee.amount}</td>
         </tr>
     })}<tr>
         <td className="name"><strong>Grand Total</strong></td>
         <td className="amount"><strong>{kb.formatCurrency(grandTotal)}</strong></td>
     </tr>
 </tbody>
</table>

Rendered output is:

<table class="billing-table"><tbody><!-- react-text: 315 --><!-- /react-text --><tr><td class="name">Travel Time #1 SUVS30M</td><td class="amount">$62.50</td></tr><tr><td class="name">GST (12%)</td><td class="amount">$7.50</td></tr><tr><td class="name"><strong>Grand Total</strong></td><td class="amount"><strong>$70.00</strong></td></tr></tbody></table>

Where is react-text: 315 coming from? JSX should strip any non-explicit whitespace, shouldn't it?

Edit: Nvm. Sorry guys. data.billing_type is an empty string, causing data.billing_type && <tr>...</tr> to evaluate to an empty string. I suppose it's good that warning kicked in and told me something funky was up, so thanks for that ;)

Slapping !! before it coerce it into a bool works, but it gets pretty nasty writing conditionals in JSX this way.

@syranide
Copy link
Contributor

syranide commented Jun 8, 2016

@mnpenner I explicitly only use if ? then : else for exactly that reason, it's just less error prone overall.

@sophiebits
Copy link
Collaborator

I think we can actually relax the warning now since we don't have spans. Certainly for an empty string.

@Ajaybhardwaj7
Copy link

Ajaybhardwaj7 commented Aug 1, 2016

It took me nearly one hour until I came to this page to know about the actual cause of warning.
There were one whitespace between my and like this and here it went everything confusing to understand this kind of bug. "#text cannot appear as a child of ".
Really hard to resolve by this message.

@nirchernia
Copy link

@Ajaybhardwaj7 I had the same issue. Thanks for confirming that I wasn't crazy.

React should really consider changing the Error message of #text cannot appear as a child of... to a warning about JSX spacing. Would save people much time.

@sophiebits
Copy link
Collaborator

Okay, #7515 should clarify.

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

No branches or pull requests

9 participants