Skip to content

Validate node nesting structure #735

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

Closed
wants to merge 6 commits into from

Conversation

sophiebits
Copy link
Collaborator

This is a better version of #644. Fixes #101.

@ghost ghost assigned sebmarkbage Dec 31, 2013
sophiebits added a commit to sophiebits/react that referenced this pull request Dec 31, 2013
Fixes facebook#767. This essentially reverts 738de8c.

We could store some sort of flag to silence the console error here but since we've made significant improvements in markup wrapping, this error is fairly rare now. We'll also have validation of node structure soon in facebook#735.
@sophiebits
Copy link
Collaborator Author

@sebmarkbage Just updated with a pretty complete validateNodeNesting function.

@syranide
Copy link
Contributor

Got notified by locks in the IRC-channel, https://www.irccloud.com/pastebin/16CNdpLZ

Someone else came across it (and I think this PR is the fix?).

@sophiebits
Copy link
Collaborator Author

Yeah, this will warn when putting a <tr> directly in a <table>.

@@ -168,7 +183,7 @@ ReactDOMComponent.Mixin = {
*
* @private
* @param {ReactReconcileTransaction} transaction
* @return {string} Content markup.
* @return {string|array<string>} Content markup or list of content markup.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is always an array, right? You can just use .forEach and .join. No need for the added dependencies and extra helper module

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the case of dangerouslySetInnerHTML it's a string. I can get rid of that and make it an array always if you prefer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(@sebmarkbage Just checking in on this.)

@markijbema
Copy link
Contributor

👍 Just ran into this while playing around with React myself. Pretty sure our production code got this problem as well ;)

@zpao
Copy link
Member

zpao commented Mar 12, 2014

After talking with @sebmarkbage, I'm going to close this out since I think we don't want to do this this way. There's work being done to make this possible without inspecting markup.

@zpao zpao closed this Mar 12, 2014
@mikemorton
Copy link

I just wanted to throw in my two cents that I came across this issue today as a react n00b and would've been completely lost without this post from @spicyj I found on the google group. I'm not sure how high of a priority this kind of a warning is but it is helpful to people who are new to the project.

@syranide
Copy link
Contributor

@mikemorton If #1550 is agreed upon then it should provide us with the ability to cheaply warn/throw when the browser mucks with the DOM.

sophiebits added a commit to sophiebits/react that referenced this pull request Mar 20, 2015
Nicer version of facebook#644 and facebook#735. Fixes facebook#101. Context is neat.
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

Successfully merging this pull request may close these issues.

React should throw on nested <p> tags
6 participants