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

ReactDOMComponent should throw error when provided children for void elements #3372

Merged
merged 1 commit into from Jan 29, 2016

Conversation

Projects
None yet
4 participants
@jonhester
Copy link
Contributor

commented Mar 11, 2015

Fix for #3367. I removed the ``if(DEV)` block since it's throwing an error now and updated the tests.

@syranide

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2015

Putting all children-logic (createContentMarkup, updateDOMChildren, etc) behind the same check should make sense I think, no point in going through the children-logic if there cannot be children.

PS. Or do we not care in the name of simplicity? cc @zpao @spicyj

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2015

@syranide Well, maybe the proposed if check should be if(props.children == null && props.dangerouslySetInnerHTML == null), since void elements don't have a monopoly on not having children.

@jonhester Thanks for fixing this one so quickly :). Just note that, as per the conversation in #3367, we need to keep it a warning for the 0.14 release, so this probably won't be mergable until 0.15

@jimfb jimfb added this to the 0.15 milestone Mar 11, 2015

@syranide

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2015

@jimfb That's tricky because you have to consider prev/next children as well and in a sense that's what the "children logic" is doing internally already, whereas void components will never have children so you can simply skip that part entirely with a single stateless check.

PS. What I'm proposing would be the equivalent of introducing a ReactDOMVoidComponent separate from ReactDOMComponent (except we don't because it's probably a little excessive and settle for a simple conditional instead).

@jonhester

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2015

@JSFB Right, not worried about when it will be merged, and I'm guessing this pull request may need some changes before it's merged, even without merge conflicts. I'm new to contributing to this project, and contributing to open source in general and have been watching the good-first-bug label to try to get some familiarity with the project.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2015

@syranide Ah, absolutely correct. If such a check is added (to this commit or a future commit), @syranide's logic is correct, we can't just check for the current presence of children, but we could do a void-element check. Probably wouldn't be blocking, but I'm a little curious what @zpao @spicyj would think about the performance vs. simplicity trade-off.

@jonhester Ok, awesome! Glad to have you be a part of the community! Let me know if there is anything we can do to help!

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2015

@jonhester We're merging 0.15 stuff now. As expected, there are a few merge conflicts. Do you want to rebase and we can get this PR ready to be merged?

@jonhester

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2015

@jimfb I'll take care of this now.

@jonhester

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2015

@jimfb Good to go.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 5, 2015

@jonhester updated the pull request.

jimfb added a commit that referenced this pull request Jan 29, 2016

Merge pull request #3372 from jonhester/void-elements
ReactDOMComponent should throw error when provided children for void elements

@jimfb jimfb merged commit 9d5825c into facebook:master Jan 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.