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

Add ReactDOM.hydrate() as explicit SSR hydration API #10339

Merged
merged 4 commits into from
Aug 1, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 1, 2017

First attempt at #10189.

  • Removes data-reactid from new SSR.
  • We now use presence of data-reactroot as heuristic for hydration.
  • There is also an explicit ReactDOM.hydrate() API that never clears the existing content.
  • ReactDOM.render() will now print a deprecation message when it tries to reuse markup.
  • Downgrade deprecation message to lowPriorityWarning.
  • A warning if we tried to hydrate but there’s no DOM to hydrate into.

This means ReactDOM.render() would still attempt to hydrate when there’s an element root, but might not work for new features (like top level strings or fragments). ReactDOM.hydrate() would always hydrate.

I have switched the integration test to use the new hydration API when available, and this let me remove the special case for top level text nodes that I added in #10333.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 1, 2017

Updated to add deprecation messages and expanded tests for both APIs while we support them.

@gaearon gaearon force-pushed the explicit-hydrate-api branch 2 times, most recently from 6ac1772 to 4096c75 Compare August 1, 2017 14:36
{'Hello ' + this.props.hello}
</body>
</html>
describe('with old implicit hydration API', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I copy pasted these tests into two separate groups.

One still calls render() and, for cases where markup is reused, we verify that in Fiber mode we print a deprecation. We can delete this group of tests in 17.

Another calls hydrate() and we don’t expect deprecations there. The new version also removes non-Fiber branches since hydrate() only exists in Fiber.

@@ -305,7 +305,6 @@ function createOpenTagMarkup(
if (isRootElement) {
ret += ' ' + DOMMarkupOperations.createMarkupForRoot();
}
ret += ' ' + DOMMarkupOperations.createMarkupForID('');
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we remove this then we break existing usage of ReactDOM.render and have no way of showing the warning, no?

Copy link
Collaborator Author

@gaearon gaearon Aug 1, 2017

Choose a reason for hiding this comment

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

Isn't the data-reactroot attribute enough for detection?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this changes the behavior if you try to ReactDOM.render into a non-container. But maybe that's fine?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is a non-container?

Copy link
Collaborator

Choose a reason for hiding this comment

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

A child of the container. The React root is the first child of the container. It also has children. You're not supported to render into those.

@bvaughn bvaughn mentioned this pull request Aug 1, 2017
Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

I'm too hungry to think about what happens with non-containers. It's probably fine because you'd have a warning in 15.

@gaearon gaearon merged commit f8062df into facebook:master Aug 1, 2017
warning(
false,
'hydrate(): Expected to hydrate from server-rendered markup, but the passed ' +
'DOM container node was empty. React will create the DOM from scratch.',
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 1, 2017

Choose a reason for hiding this comment

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

IMO we should revert this warning since it's valid to have an empty container if your server rendered "", null, false or [] at the root. You'll get a warning about hydration being a non-match later anyway, if it's not. We can potentially special case the bulk error message in #10085 if the only diff was that the container was empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah okay. Will do.

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

Successfully merging this pull request may close these issues.

3 participants