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

Ignore whitespace in the container element when reusing markup #996

Closed
Rowno opened this Issue Jan 30, 2014 · 11 comments

Comments

Projects
None yet
5 participants
@Rowno

Rowno commented Jan 30, 2014

At the moment, if there's any whitespace around a pre-rendered component inside it's container, the HTML won't be reused. This is because getReactRootElementInContainer() uses Node.firstChild which will return a TEXT_NODE instead of an ELEMENT_NODE in this case.

For example, this HTML won't be reused:

<div class="wrapper">
    <div class="component" data-reactid=".r[1wtmm]" data-react-checksum="490228448">...</div>
</div>

If you want, I can send through a pull request that changes getReactRootElementInContainer() to look for the first ELEMENT_NODE instead of just any Node.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 30, 2014

My inclination would be to ignore only text nodes comprised solely of whitespace.

@Rowno

This comment has been minimized.

Rowno commented Jan 30, 2014

True, that will work too.

@syranide

This comment has been minimized.

Contributor

syranide commented Jan 30, 2014

This is fixed in master (my whitespace PR took care of spaces appearing literally everywhere).

@Rowno

This comment has been minimized.

Rowno commented Jan 30, 2014

Sweet, looking forward to the next release then. 😀

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 30, 2014

@syranide I thought your PR only modified jsx -- this is about adopting server-rendered markup.

@syranide

This comment has been minimized.

Contributor

syranide commented Jan 30, 2014

@spicyj Yes it's only JSX. It seemed to me like white-space (arbitrarily) introduced by JSX was the issue. But perhaps I'm mistaken? Proof-reading it again, it may be the case actually (not 100% sure what @Rowno means :)).

@Rowno

This comment has been minimized.

Rowno commented Jan 30, 2014

Maybe this is clearer.

This markup won't be reused:

<div class="container">
    <div class="component" data-reactid=".r[1wtmm]" data-react-checksum="490228448">...</div>
</div>

But this markup will:

<div class="container"><div class="component" data-reactid=".r[1wtmm]" data-react-checksum="490228448">...</div></div>

The .component element is the output generated by React.renderComponentToString(). I'm not using React to render the entire page, only the dynamic parts. So this problem occurs when combining the React output with other server-side templates.

@syranide

This comment has been minimized.

Contributor

syranide commented Jan 30, 2014

@Rowno Aha, yeah my mistake then. Not fixed in master.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Feb 13, 2014

(petehunt says: "We just need to change our use of firstChild in src/browser/getReactRootElementInContainer.js to be children[0].")

@JannesMeyer

This comment has been minimized.

JannesMeyer commented Jun 18, 2014

I experimented with a possible fix a little bit. As @spicyj said in #1050 it might be worth it to not ignore prepended non-whitespace text.

This gist would be the necessary code if you want to make sure you only ignore text nodes that consist purely of whitespace. But this would get rather more complex if you wanted to make sure you account for the edge case of alternating comment nodes and text nodes until you actually find an element node or a text node with non-whitespace content.

Therefore I think it would be much easier to implement @petehunt's proposal and just use container.children[0].

In my opinion it makes no sense to do this non-whitespace check, because React doesn't actually make sure there is no appended non-whitespace text neither when re-using DOM nodes that came from the server.

@aweary

This comment has been minimized.

Member

aweary commented Jul 12, 2017

Closing this out since #1912 implemented a warning that would cover this situation (exampe)

@aweary aweary closed this Jul 12, 2017

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