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

Use ReactDOM.hydrate() for hydrating a SSR component if available #1028

Merged

Conversation

theJoeBiz
Copy link
Contributor

@theJoeBiz theJoeBiz commented Feb 9, 2018

Looks like this was originally addressed in #1013, but was closed by accident and never reopened. My project is using React 16 and I can confirm that this patch prevents the deprecation warning.


This change is Reviewable

@coveralls
Copy link

coveralls commented Feb 9, 2018

Coverage Status

Coverage remained the same at ?% when pulling 9e86b75 on theJoeBiz:fix-react-render-deprecation-warning into eb4438a on shakacode:master.

@justin808
Copy link
Member

@theJoeBiz Any way to test or verify this for both older React and new react?

@theJoeBiz
Copy link
Contributor Author

@justin808 I don't think so. There aren't any existing tests for clientStartup.js and I've never had to deal with tests for two separate versions of react. That being said, the change made here is fairly transparent and visibly falls through to the old behavior for any version (older than 16) where ReactDOM.hydrate is undefined.

@theJoeBiz
Copy link
Contributor Author

Also, only one test needs to be updated for a react 16 upgrade
theJoeBiz@468338c#diff-85a5145721dbc772cfb6de2e6102717c

@justin808
Copy link
Member

@alexfedoseev can you please confirm this React syntax looks good.

ReactDOM.hydrate &&
domNode &&
domNode.hasAttribute('data-reactroot')
);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you mix things here you shouldn't mix. E.g. !!ReactDOM.hydrate is enough for shouldHydrate decision, but if something is wrong w/ dom node then it's out of scope ReactDOM.hydrate vs ReactDOM.render problem.

Also, facebook/react#10971

@theJoeBiz theJoeBiz force-pushed the fix-react-render-deprecation-warning branch from 51b40de to 0bf1415 Compare February 12, 2018 14:46
@theJoeBiz
Copy link
Contributor Author

@alexfedoseev Good call, updated to just check for truthiness of ReactDOM.hydrate.

@justin808
Copy link
Member

@alexfedoseev let me know if I should merge.

Copy link
Member

@alex35mil alex35mil left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@theJoeBiz
Copy link
Contributor Author

theJoeBiz commented Feb 22, 2018

We just moved our app into production with this warning, which is fine, but I was just revisiting this on a page that has a combination of server/client rendered components and found a flaw. Turns out, the data-reactroot check I removed in a previous iteration of this was there to check to see if the component was server rendered. In react 16, ReactDOM.hydrate is only supposed to be used on components that were previously rendered on the server. So, now that data-reactroot is unreliable, I replaced that check with !!domNode.innerHTML so the hydrate check is now:

const shouldHydrate = !!ReactDOM.hydrate && !!domNode.innerHTML;

I tested this most recent commit locally on a page using React 16 and a combination of server/client rendered components. Here is the output from the console:

(index):68 [SERVER] RENDERED SearchForm to dom node with id: SearchForm-react-component with railsContext: {...,"serverSide":true}
(index):98 [SERVER] RENDERED Explore to dom node with id: Explore-react-component with railsContext: {...,"serverSide":true}
createReactElement.js:41 HYDRATED SearchForm in dom node with id: SearchForm-react-component using props, railsContext: {} {…,"serverSide":false}
createReactElement.js:41 HYDRATED Explore in dom node with id: Explore-react-component using props, railsContext: {} {…,"serverSide":false}
createReactElement.js:43 RENDERED FreeReport to dom node with id: FreeReport-react-component with props, railsContext: {} {…,"serverSide":false}
createReactElement.js:43 RENDERED ClientOnlyTest to dom node with id: ClientOnlyTest-react-component with props, railsContext: {} {…,"serverSide":false}

@justin808
Copy link
Member

@alexfedoseev Please look at the latest change. I know we want to use this as well.

@justin808
Copy link
Member

@theJoeBiz can you please rebase on top of master and add an entry to the CHANGELOG.md

@theJoeBiz theJoeBiz force-pushed the fix-react-render-deprecation-warning branch from 60e3ddf to 89fde6a Compare February 23, 2018 15:40
@theJoeBiz theJoeBiz force-pushed the fix-react-render-deprecation-warning branch from d87313b to 9e86b75 Compare February 23, 2018 15:47
@theJoeBiz
Copy link
Contributor Author

@justin808 added an entry to CHANGELOG.md and squashed the commits

@alex35mil
Copy link
Member

LGTM

@justin808 justin808 merged commit 7e18fef into shakacode:master Feb 28, 2018
@justin808
Copy link
Member

Shipped! 10.1.2

@theJoeBiz
Copy link
Contributor Author

Any estimate on getting this onto npm as 10.1.2?

@justin808
Copy link
Member

@theJoeBiz it should have published. I'm looking into it.

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