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

Two-pass render on the client #81

Closed
amannn opened this issue Feb 5, 2018 · 13 comments
Closed

Two-pass render on the client #81

amannn opened this issue Feb 5, 2018 · 13 comments
Assignees

Comments

@amannn
Copy link

amannn commented Feb 5, 2018

Thanks for this great library!

I recently had an issue while rehydrating server side rendered markup that contained a react-media component. I'm pretty sure it was because the server side rendered markup was the desktop version and on the client the component immediately rendered the mobile markup when the respective media query matches.

With React 16 it's no longer mandatory to have a 100% matching markup when rehydrating; React will try to patch the DOM as best as it can. In practice this can lead to some problems though if the DOM is really different – resulting in an incosistent DOM that is a mix of the desktop and the mobile markup.

The React docs suggest a two-pass render for such scenarios that will avoid the problem:

If you intentionally need to render something different on the server and the client, you can do a two-pass rendering. Components that render something different on the client can read a state variable like this.state.isClient, which you can set to true in componentDidMount(). This way the initial render pass will render the same content as the server, avoiding mismatches, but an additional pass will happen synchronously right after hydration. Note that this approach will make your components slower because they have to render twice, so use it with caution.

Maybe it could be helpful to move calling this.updateMatches() from cWM to cDM. This is a disadvantage for clientside-only apps though, as it could add a potentially unnecessary render.

One strategy could be to remove the default for the defaultMatches prop. As this property is only needed for server side rendering, we could invoke the two-phase render only when this prop is actively provided by the user.

What do you think?

@chrisjcodes
Copy link

I have a wrapper around react-media that I use to make sure we are only using it for defined breakpoints in our CSS and for two pass rendering. Seeing as cWM will be deprecated in the future I think this would have to be done anyway.

import React, { Component } from 'react';
import PropTypes from 'prop-types';
import _keys from 'lodash/keys';

import Media from 'react-media';
import breakpoints from 'style-utils/breakpoints.scss';

const breakpointsMap = {
    'phone-only': { maxWidth: breakpoints.phoneonly },
    'tablet-portrait-up': { minWidth: breakpoints.tabletportrait },
    'tablet-landscape-up': { minWidth: breakpoints.tabletlandscape },
    'desktop-up': { minWidth: breakpoints.desktop },
    'big-desktop-up': { minWidth: breakpoints.bigdesktop },
};

class Breakpoint extends Component {
    state = { isClient: false };

    componentDidMount() {
        this.setState({ isClient: true });
    }

    render() {
        const { name, ...props } = this.props;
        const { isClient } = this.state;

        return isClient && <Media {...props} query={breakpointsMap[name]} />;
    }
}

Breakpoint.propTypes = {
    name: PropTypes.oneOf(_keys(breakpointsMap)).isRequired,
};

export default Breakpoint;

@owenhoskins
Copy link

@unruffledbeaver I am trying to follow the logic of this wrapper but wouldn't the component never server-render when isClient will always be false server-side?

@chrisjcodes
Copy link

chrisjcodes commented May 9, 2018

@owenhoskins You could modify my implementation to use defaultMatches to always render that markup on the server if that’s a concern. If you know your user base well you can assume a device based on common usage. If not then I don’t see why bother rendering any markup server side at all until you know what to render

@owenhoskins
Copy link

@unruffledbeaver Thanks for the reply! I've set defaultMatches to true but the isClient check seems to prevent the server rendering because componentDidMount won't run server-side as I understand it.

For my use-case the markup rendered server side would be a speed-boost and most importantly for for crawler bots.

@chrisjcodes
Copy link

chrisjcodes commented May 9, 2018

@owenhoskins You are correct, in my component, it never renders on the server.

SSR will definitely improve your time to first meaningful paint. As far as crawlers, most if not all major ones can parse single page apps that are purely client rendered so I don't think this will be a huge concern. The bigger benefit of SSR for crawlers, IMO, is proper meta data for individual routes.

I use this approach for media queries very sparingly, only when the markup absolutely has to change based on breakpoint and I use it around the smallest piece of the markup possible. If its possible to use CSS to achieve what I need, I don't bring this in. In fact I left out the comment I have at the top of this component warning others of the tradeoffs in using it:

/*
 This component is using two-pass rendering: https://reactjs.org/docs/react-dom.html#hydrate
 Use this component ONLY in rare scenarios where markup must change based on viewport size  

 Favor CSS media queries if possible.
 */

@theKashey
Copy link

Not rendering anything... it does not sound right.

The better way - render something, your most used media for example, and ship it to client. If the prediction was wrong - use react-dom.render, not rehydrate.

@chrisjcodes
Copy link

chrisjcodes commented Jun 28, 2018

@theKashey see my comment above, I already mentioned that you should send back a default if you have a sensible one. We rarely use viewport to determine markup so in our use cases the render has to drastically change between viewports which usually doesnt yield a very good default.

@edorivai
Copy link
Collaborator

we could invoke the two-phase render only when this prop is actively provided by the user.

I really like this 👆, seems like best of both worlds to me. And yeah, as was mentioned earlier here, cWM will be deprecated, and as far as I can tell, the recommendation from the React team is to move this.updateMatches to cDM.

@mjackson Would you be open to this proposal?

@edorivai
Copy link
Collaborator

edorivai commented Sep 3, 2018

@amannn I've submitted #96 which implements the mechanism you proposed. Unfortunately I don't know yet when we'll be able to merge and publish. Please be patient 🙏

edorivai added a commit to edorivai/react-media that referenced this issue Sep 3, 2018
@junemoretz
Copy link

@edorivai I've tested your changes out on my application and it doesn't appear that the two-pass render is working properly. I'm experiencing the same behavior as before this change (with a value set for defaultMatches, so it should be running). Not sure why this would be happening or how best to resolve this behavior, any ideas as to why this might not be running?

@edorivai
Copy link
Collaborator

@kmoretz How did you test this out? It would be really helpful if you could share a minimal reproduction repo, so I can take a look 🙏

@junemoretz
Copy link

@edorivai I just tried to put together a minimal repo for this but had some issues reproducing the problem I had on your branch outside of our codebase. I've worked around this for now using some media queries but I'll put together a demo if I keep experiencing the same issue whenever this is merged.

@edorivai edorivai mentioned this issue Dec 12, 2018
8 tasks
edorivai added a commit that referenced this issue Dec 23, 2018
Implement two-pass render (closes #81)
@edorivai
Copy link
Collaborator

This should already be available in the prerelease of 2.0.0-alpha.1. Please beware that the versioning is still up for debate; there is a good chance that this fix for this issue will actually land in a minor (v1) update.

You can expect the release soon (see #123 for details)!

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

No branches or pull requests

6 participants