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

Document that you can't rely on React 16 SSR patching up differences #10591

Closed
Tarnadas opened this issue Sep 1, 2017 · 19 comments
Closed

Document that you can't rely on React 16 SSR patching up differences #10591

Tarnadas opened this issue Sep 1, 2017 · 19 comments
Assignees

Comments

@Tarnadas
Copy link

Tarnadas commented Sep 1, 2017

With SSR being loaded on client side, there are various wrong behaviors if the server's HTML differs from the client's HTML.

For a minimal example, I created this repository.
Here is a code snippet:

class AppView extends React.Component {
  render () {
    const isServer = this.props.isServer
    const styles = {
      server: {
        backgroundColor: 'red'
      },
      client: {
        backgroundColor: 'green'
      }
    }
    return (
      <div>
        {
          isServer ?
            <div style={styles.server}>isServer</div> :
            <div style={styles.client}>isClient</div>
        }
      </div>
    )
  }
}

In the example I render a CSS background color of red for the server and green for the client. I force a difference by the server's and client's HTML with the isServer property.

With React 15, everything works as expected: the server renders a red background, but the client corrects it to green. With React 16 however the background stays the same, but the text changes as expected.

There are probably other similar behaviors. For example I found out about this bug, because I was conditionally rendering a complete component like so:

return someCondition && <MyComponent />

It becomes even more weird, because if there is additional JSX after that conditional rendering, it would render that additional JSX as if it is inside that MyComponent

return (
  <div>
    { someCondition && <MyComponent /> }
    <SomeOtherComponent />
  </div>
)

becomes

return (
  <div>
    <MyComponent>
      <SomeOtherComponent />
    </MyComponent>
  </div>
)

if someCondition === true on server side and someCondition === false on client side.

You can see this behavior on my website: http://smmdb.ddns.net/courses
Open Chrome Dev Tools and lower the width until you get the mobile view, then reload the page and see how the list is wrapped inside another Component.

@sebmarkbage
Copy link
Collaborator

The styles not being patched up is expected. That's an intentional breaking change. The idea behind that is that most sites stabilize once they have a solid SSR infra in place and therefore these things don't happen, yet they would have to spend all that time validating unnecessarily when they know it's not going to change. So it's an optimization.

The second part here should not happen though. It should not be inserted inside another parent. Do you think you'd be able to provide a smaller reduced example e.g. in a JSFiddle? It's a bit difficult to debug on a fully deployed site.

@sebmarkbage
Copy link
Collaborator

In development mode you should see a warning, btw. Are you not seeing the warning?

@Tarnadas
Copy link
Author

Tarnadas commented Sep 5, 2017

I updated the repo with an example of what I mean.

Here is a code snippet:

import React from 'react'

export default function renderer (isServer, render) {
  return render(<AppView isServer={isServer} />, !isServer && document.getElementById('root'))
}

class AppView extends React.Component {
  render () {
    const isServer = this.props.isServer
    return (
      <div>
        {
          isServer && <ServerComponent />
        }
        <ListComponent />
      </div>
    )
  }
}

class ServerComponent extends React.Component {
  render () {
    const styles = {
      server: {
        backgroundColor: 'red'
      }
    }
    return <div style={styles.server} />
  }
}

class ListComponent extends React.Component {
  constructor (props) {
    super(props)
    this.getList = this.getList.bind(this)
  }
  getList () {
    const styles = {
      el: {
        width: '200px',
        height: '200px',
        margin: '20px',
        backgroundColor: 'blue'
      }
    }
    return Array.from((function * () {
      for (let i = 0; i < 10; i++) {
        yield <div style={styles.el} key={i} />
      }
    })())
  }
  render () {
    const styles = {
      client: {
        backgroundColor: 'green',
        display: 'flex',
        flexWrap: 'wrap'
      }
    }
    return (
      <div style={styles.client}>
        { this.getList() }
      </div>
    )
  }
}

And this will happen:

ServerComponent should get removed, but it won't. Instead it stays and the ListComponent's list elements get transferred to the <div> from ServerComponent. The surrounding <div> from ListComponent will be removed/replaced by the <div> from ServerComponent.

So you said the styles won't be patched, which is an optimization, but in this case I replace an entire div so shouldn't this update the style or is it WAI?

The problem on my website is that I make assumptions about screen size, which might not be true. Is there a stable method to get screen size on initial server rendering? Can I use the newly introduced SSR streaming method to get this working?

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Sep 5, 2017

Ah. The reason this happens is because we try to reuse the structure as much as possible. Because the first item is a div in both cases we try to reuse it. That's why it retains its styles. If it was a different tag name it would patch it up but it's risky to rely on this.

You can build a little infra that passes screen size to the server as a separate request but that might be overkill. If you don't have screen size on the server you will server render the wrong output which could lead to flashes on the content when it finally restores the JS on the client. If your app is resilient to that, then there is a way to model that on the client.

You can render the initial mount with the server render screen size. Then in componentDidMount you can read the real screen size and if it doesn't match up with your guess, then you rerender by setting state.

class App extends React.Component {
  state = { screenSize: DEFAULT_SIZE };
  componentDidMount() {
    this.setState({ screenSize: getRealScreenSize() });
  }
}

@rgrove
Copy link

rgrove commented Sep 5, 2017

I also have a use case in which I intentionally have the server render something slightly different than the client will render, and it's affected by this change in React 16 as well.

I have a component that renders a feed of items. On the server I render one page of items at a time, with conventional "Prev" and "Next" pagination links. On the client I render an infinitely scrollable virtualized list.

Rendering a conventional paginated list on the server helps improve SEO by giving search bots actual links to follow to subsequent pages and also ensures that clients with JS disabled can still paginate through the list.

There's no visible flash when a JS-capable client hydrates the list because the only visible difference (pagination links) will typically be below the fold, so this should work out well. However, React 16 reuses certain server-rendered elements without removing their classNames even though the client-rendered state doesn't use those classNames, and this breaks the design in weird and unexpected ways.

I understand that it's generally not advisable for server-rendered content to differ from client-rendered content, but this was a conscious choice made with full awareness of the disadvantages. I expected a slight performance penalty, but didn't expect broken output.

If this scenario really is considered unsupported in React 16 and there are no plans to allow developers to opt into full validation when hydrating server HTML, then it seems like the dev mode warning should be an outright error in order to prevent subtle breakage from slipping into production. This change should probably also be called out prominently in the release notes, since it could unexpectedly break existing code that relies on the old behavior.

@sebmarkbage
Copy link
Collaborator

But, yea this will definitely need to be prominent release note and maybe a separate blog post.

What's your mechanism for silencing the warning in 15? Maybe we can target that as a way to inform you?

We rely pretty heavily on console.error being considered an actual error, and can even rely on it between minor versions to change the behavior - since it was a true error to begin with. We don't throw because that causes even more potential problems when code paths differ between development and production. There are really unexpected ways. Yea, I know this is a weird Reactism.

Note that hydration isn't just a performance optimization but a way to preserve state in things like forms and animations.

I'm going to add a way to suppress the diff warning for individual nodes. At least for text content differences. We could potentially expand that to direct child elements of that node too. That way you could work around it by using different tag names on the container element.

@rgrove
Copy link

rgrove commented Sep 6, 2017

What's your mechanism for silencing the warning in 15? Maybe we can target that as a way to inform you?

Basically I close my eyes and put my fingers in my ears and yell "you're not my supervisor!" 🙈

I took another pass at my list component using the two-phase render approach you recommended above and it works well, so I think that'll be good enough to cover my use case. Thanks!

My guess is I'm not the only one who's gotten used to ignoring that warning, so I'll try to help get the word out about the change.

@Tom910
Copy link
Contributor

Tom910 commented Sep 6, 2017

My case of use. Our server does not know about authorization. And when transitions between applications, we quickly restore part of the data of the authorized zone. In this case, patch html and story. Thus, there is a fast switching between applications.

@flarnie flarnie self-assigned this Sep 6, 2017
@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Sep 6, 2017

Btw, if you relied on React to clear out the DOM for you in 15 you can still do that explicitly in 16. Just do container.innerHTML = '' before ReactDOM.render. (In 17, ReactDOM.render will do that automatically but currently it doesn't for backwards compat.)

@flarnie
Copy link
Contributor

flarnie commented Sep 11, 2017

It sounds like the action item here is to update docs/release notes and possibly write a blog post about this change, showing people how to migrate their code if they run into this issue.

@zombieJ
Copy link
Contributor

zombieJ commented Sep 27, 2017

hi @sebmarkbage,
We use react-hot-reload + webpack-hot-module-reload on ssr. This works find in 15.5 and front end always sync with latest version. But for v16, I have to keep the server side code update to date to avoid dom element over-reuse.
container.innerHTML = '' could help but something we finished a page and wish the warning in console of c-s diff. innerHTML = '' & render will break the warning.
I think it could be a hook of the react client side render listener for c-s diff. When trigger, just let me reload all the page by client side.(Maybe already have? Not sure)

@eteeselink
Copy link

@sebmarkbage, thanks for your thorough answers here. We're running into this too and your comments helped us understand what's going on.

That said, in all honesty, I don't completely see how this is not considered a bug. It's very hard and messy to ensure that what's rendered server-side and client-side are 100% equal. For instance, what if a component displays something time-based and the client clock isn't millisecond-precise equal to the server clock? (e.g. always). In our case, seemingly random (but consistent) items in a list suddenly get some child div content swapped.

Obviously everything can be worked around by copying all and every piece of global state from the server to the client (and even ensuring that eg the same millisecond time is used in the entire render call), but isn't that major overkill?

Obviously, by using (new Date()).getTime() as an input, our components aren't 100% purely functional, but making React completely break because components aren't 100% purely functional is quite a stretch :-) I'm sure there's other kinds of singleton-ish data than time that exhibit a similar dynamic.

(in all honesty I think the screen size example from @Tarnadas is a nice example too)

@sebmarkbage
Copy link
Collaborator

@eteeselink Typically time based things are only things like time stamps where only the text changes, not the order of items. Those will work fine since the text content in those will be patched up. We're also planning on adding the ability to ignore the warning in that specific special case.

A rare but plausible scenario is that you have a list or something that depends on time to determine the order. E.g. something that conditionally pushes something on the top of a list if some time has elapsed. Or changes the the class name based on time elapsing. These cases are very theoretical though. I haven't seen one yet. You probably want to represent these as some kind of global data state change or transition anyway. E.g. to indicate that an alarm was fired so you know to ignore it. Even in the worse case scenario you don't need to send all state down to solve time based issues, use virtualized time.

Note that if you use the "current" time you can get "tearing" across the screen even if you're only rendering on the client since not all components render at exactly the same time, they can end up with different time stamps for the same render. This can be more prevalent with async rendering where the time to render an entire update can be longer.

A bigger issue would be that data fetching from backend end points can return different data. That's why most systems that use React server rendering seriously send the data along with the initial request anyway. E.g. see Next.js's getInitialProps mechanism.

This avoids two trips to backend services which avoids unnecessarily hitting those services with 2x the ratio. You already have the data loaded so you might as well tack it on to the end of the initial request pipe. I agree that this is quite complex but it seems like it is ultimately the best architecture for other reasons.

Now, in theory we could go back to the model of comparing everything and trying to patch things up. That would make things significantly slower for everyone that does the above architecture anyway for no reason. In theory we could have two modes.

However, even in that mode, you'll still have cases where state such as text input and scroll position changes unexpectedly to the user as the result of inconsistent data. This also doesn't seem ideal.

@gaearon gaearon changed the title React 16 SSR - hydration on client not working if server html !== client html Document that you can't rely on React 16 SSR patching up differences Oct 4, 2017
@thebuilder
Copy link

Also ran into this issue after upgrading to React 16. Using react-mediai was intentionally rendering out different classNames based on the viewport size, and using CSS media queries to hide the the default HTML output on desktop.

This broke after upgrading, since it would render the mobile view with the desktop class. My first idea to solve this was to add different key values to the <div>, but this didn't have any effect.

Basic example:

<Media query="(min-width: 1025px)">
        {matches =>
          matches ? (
            <div className="desktop">Desktop size</div>
          ) : (
            <div className="mobile">Mobile size</div>
          )}
</Media>

I solved it for now by using the same CSS class and using media queries to modify the container.

@gaearon
Copy link
Collaborator

gaearon commented Oct 5, 2017

@thebuilder

If you want to render something different on the client, you can set state in componentDidMount and do it there. #8017 (comment)

@dustinsoftware
Copy link
Contributor

Thanks for the fast response! I see now that this behavior was called out in What's New With Server-Side Rendering.

I have a couple suggestions, both of which I'd be willing to open a PR for:

  1. The React 16 blog post calls out that mismatches are not recommended, but indicates that some changes are OK (e.g. timestamps). This page, and the React docs, could be updated to point out that it's dangerous to have missing nodes on the server render, and they might get re-created with incorrect attributes.

  2. There used to be client-side warnings when server-render did not match what the client expected. I can see the warnings being unhelpful with innerText changes in a single DOM node, but if an entire node is missing, it would be helpful to show a warning, since this case is unsupported. Warning: <div className='two'> was missing during server-render, it might have been recreated with incorrect attributes

@bvaughn
Copy link
Contributor

bvaughn commented Oct 6, 2017

Thank you for filing this issue! 😄

The documentation and source code for reactjs.org now lives in a different repository: reactjs/reactjs.org. (For more info on why we made this move, see issue #11075.)

This issue is a bit tricky because it seems like there are a couple of calls to action.

For the documentation bit, I've created an issue in the new repo: reactjs/react.dev#25

Someone (@dustinsoftware?) should open a new issue for the proposed warning message modifications so we can continue the discussion there.

I will close this issue out but I'll be sure to reference the discussion thread above in the new issue.

@johnnyreilly
Copy link

johnnyreilly commented Nov 15, 2017

I've just stumbled on this. In my own case, my app essentially just renders tons of anchor tags.

However, post hydrate I'm left with the case where a links inner text may have updated, but the href etc has not. So whilst the server sends this:

<li>
  <a href="http://go.theregister.com/feed/www.theregister.co.uk/2017/11/14/fcc_commissioner_continues_pushing_for_puerto_rico_hearings/" 
     class="story-title" 
     data-title="FCC commish wants more than one-page updates on recovery Jessica Rosenworcel, one of the commissioners at America&amp;#039;s broadband watchdog the FCC, has reiterated her call for hearings into what is happening with communications on the hurricane-stricken island of Puerto Rico.…"
  >How about that US isle wrecked by a hurricane, no power, comms... yes, we mean Puerto Rico</a>
</li>

Post-hydrate we have a link that points to a different location than suggested by the inner text:

<li>
  <a href="http://go.theregister.com/feed/www.theregister.co.uk/2017/11/14/oneplus_backdoor/" 
     class="story-title" 
     data-title="Who left &amp;#039;wipe the engineering toolkit&amp;#039; off the factory checklist? Updated An apparent factory cockup has left many OnePlus Android smartphones with an exposed diagnostics tool that can be potentially exploited to root the handsets.…"
  >How about that US isle wrecked by a hurricane, no power, comms... yes, we mean Puerto Rico</a>
</li>

For users this is obviously deeply confusing! It feels like this kind of failed reconciliation is a real issue. At the moment it seems that the only way to get around this is to stop rendering links / anchor tags on the server entirely. That will force the client to render them correctly.

This rather removes the benefit of SSR though (at least for my own case where the app is not much more than a glorified collection of links).

My takeaway from this is that rendering lists of links / anchor tags with SSR may then leave you with links that point to different locations when hydrate takes place.

Could you confirm that's intended behaviour please? I just want to be sure I haven't misunderstood the intention of this change. In case it's relevant the app is here and the source code is here.

@gaearon
Copy link
Collaborator

gaearon commented Nov 15, 2017

React does not introduce any differences on its own, whether in text content, anchor tags, or anywhere else.

But if your components return different things on client and server, React won't attempt to fix the difference.

You are expected to return the same thing on the client and the server. Otherwise it's a bug in your app. React warns you about mismatches in development, and you should fix if you see any.

This thread is getting long and I don't think it's a good place to track any new problems. If you believe that what you see might be a bug in React please file an issue. The hydrate() behavior is now well documented so I'm going to lock discusssion on this one to prevent further confusion.

https://reactjs.org/docs/react-dom.html#hydrate

@facebook facebook locked and limited conversation to collaborators Nov 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests