search

artsy / fresnel

An SSR compatible approach to CSS media query based responsive layouts for React.
https://artsy.github.io/blog/2019/05/24/server-rendering-responsively
Other
1.25k stars 65 forks source link

SSR | Render props hyration issue #184

Closed fadi-mimi closed 2 years ago

fadi-mimi commented 3 years ago

I have the same issue as #165. This ticket was closed but actually this is a serious hydration error. Because the renderChildren is true on the server and false on the client for the specified breakpoints. This leads to a is a mismatch. As pointed out in the other issue in the component this is working fine. The warning is not shown in production because React suppresses all the warnings there.

The solution should be that renderChildren is always true on the hydration of the client and then turns false in the second step if the breakpoints do not match.

damassi commented 3 years ago

Hi @fadi-mimi, thanks for the issue. If you have a moment to put together a PR with a solution that would be greatly appreciated 🙏

fadi-mimi commented 3 years ago

I made a PR to reproduce the issue. The test was already there and skipped. There was the problem with the breakpoint which was mocked before the server rendering. But this is not the case in reality since these are just defined on the client. So I did the mocking after the server rendering and the error occurs as expected. I also put the snapshots there so you can see the difference between server and client markup. React treats this as a hydration error.

This is quite a huge change architectural wise and I don't have the feeling that I'm the guy to decide this.

saeedPadyab commented 3 years ago

I made a PR to reproduce the issue. The test was already there and skipped. There was the problem with the breakpoint which was mocked before the server rendering. But this is not the case in reality since these are just defined on the client. So I did the mocking after the server rendering and the error occurs as expected. I also put the snapshots there so you can see the difference between server and client markup. React treats this as a hydration error.

This is quite a huge change architectural wise and I don't have the feeling that I'm the guy to decide this.

Hey @fadi-mimi hope you'r doing well. any news from your PR. I hardly need your PR if it is solved the miss-match issue.

damassi commented 3 years ago

Hey all, sorry for the delay. Things are very busy over here right now. We'll happily accept PRs to fix issues but unfortunately, short of that, this issue will have to wait.

engineerahkhani commented 3 years ago

Hey @fadi-mimi, the Same issue for me, do we have to wait long?

saeedPadyab commented 3 years ago

Hey all, sorry for the delay. Things are very busy over here right now. We'll happily accept PRs to fix issues but unfortunately, short of that, this issue will have to wait.

thanks for your respond quickly. to be honest i am going to use common way css display property if it is going to take more than one week. what is your idea?

Godsenal commented 3 years ago

I think <Media /> component set suppressHydrationWarning = true to wrapper element because of this kind of warning. But when use renderProps, <Media /> doesn't render any wrapper element so can't set suppressHydrationWarning.

https://github.com/artsy/fresnel/blob/master/src/Media.tsx#L470-L476

The solution should be that renderChildren is always true on the hydration of the client and then turns false in the second step if the breakpoints do not match.

I think this might be cause side-effects of the child components. If we render child component on hydration, those components all hooks and lifecycle will be called once (which is not expected).

My workaround is to put suppressHydrationWarning on the custom wrapper element.

fadi-mimi commented 3 years ago

Setting suppressHydrationWarning is of course no solution, but just hides the problem. The solution is to introduce a state in the component that is set to true on mount and only then hides the elements. I will work on a PR for this.

cherryrap commented 2 years ago

Does anyone know if the PR was introduced?

damassi commented 2 years ago

See this note.

Tracking the issue here.