Skip to content

Conversation

@syranide
Copy link
Contributor

PR for #996

I choose to believe that users should be correcting their own "mistakes", rather than have React transparently fix and unknowingly introduce weird edge-cases and visual artifacts.

Quite simply, this PR enforces that there are no other nodes except the node rendered by React inside the target, it obviously does not warn if there isn't a node rendered by React inside the target. It only performs this check in DEV.

There is another test in #1903 which complements this PR.

@oriSomething
Copy link

what should i expect if i use server side to render version of the page which wasn't done with react that anyway is a bit different from the react server?
(not possible for me to use react in server side yet)

@syranide
Copy link
Contributor Author

@oriSomething React require identical markup to reuse server-render nodes, but that is not what this PR addresses, this is just about an edge-case some people have gotten confused by.

@sophiebits
Copy link
Collaborator

This throws if you have any existing markup in the element, regardless of if it was produced using React's server rendering? Seems a little aggressive.

@syranide
Copy link
Contributor Author

it obviously does not warn if there isn't a node rendered by React inside the target

@spicyj :)

@sophiebits
Copy link
Collaborator

Can you add a test for that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this should really be a warning since it's dev-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do, I was a bit unsure and did a quick search, found a file using invariant in dev and didn't think more about it. We really should allow people to turn some warnings into errors though, because this is a clear case of "you really don't want to do/miss this".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed (wups, gotta fix test too, EDIT: fixed).

Copy link
Collaborator

Choose a reason for hiding this comment

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

What file does dev-only invariants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@syranide
Copy link
Contributor Author

@spicyj Added.

syranide added a commit that referenced this pull request Jan 31, 2015
Warn if mounting into node with dirty rendered markup
@syranide syranide merged commit 905bfce into facebook:master Jan 31, 2015
@syranide syranide deleted the safereuse branch January 31, 2015 18:57
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.

4 participants