Effective responsiveness

[alloy]

For reasons such as efficiency (4) and SEO, we want to do server-side rendering in such a way that the response sent to the client is immediately usable without the need to re-render.

Alas our current solution, which essentially boils down to if/else branching per breakpoint, doesn’t fulfil these needs as it is practically impossible (1) to know what the breakpoint is that the client will want to display at SSR time.

Additionally, even when getting the initial breakpoint right, the performance of changing breakpoints will be worse when a browser has to evaluate JS, recalculate a component tree, diff DOM elements, and update the DOM; as opposed to the browser only needing to re-render the styling without any DOM changes. (2)

Example

Consider a desktop browser requesting a page that is rendered as follows:

<Responsive>
  {({ xs }) => {
    if (xs) {
      return <Box width={1} />
    } else {
      return <Box width={1/2} />
    }
  }}
</Responsive>

On the server we’ll render a xs version (because that’s currently our default), then once the browser receives the response it may start to render that version.

However, once React is fully booted it starts to render a virtual DOM version based on an actual media query that may result in a different breakpoint than xs. If that vDOM does not match what the server sent, the server version is thrown away and the new version is used instead.

Options

Note that these are just some bare-bones examples. Where possible it would be great to come up with prettier declarative APIs that use these techniques under the hood.

Pure CSS

In cases where all differences in layout can be expressed in styling, we should be able to do something like:

const ResponsiveBox = styled.div`
  @media screen and (max-width: 767px) {
    width: 100%;
  }
  @media screen and (min-width: 768px) {
    width: 50%;
  }
`

<ResponsiveBox />

When some components should only render on certain breakpoints, these may simply be hidden on those breakpoints using CSS. (3)

styled-system

We’re using styled-system, which out of the box comes with responsive options that, I assume–and we should verify this, do exactly what the above option does. For instance, rather than the above example, we should write <Box width={[1, 1/2]} />.

More…

Please suggest yours!

Notes

1. That is, without resorting to added complexity such as sniffing user-agent and even then it won’t be foolproof.
2. While this performance penalty is even noticeable on desktop machines, it’s even more important for mobile devices.
3. In the case of mobile devices, this may lead to documents being sent to the client that include markup that will never be used, which presumably could add up for large pages. Seeing as in some of these cases, such as iPhones, we can pretty reliably determine the client never needs larger breakpoints, it would be great to have an API that could omit certain breakpoints completely.
4. Besides the aforementioned re-rendering of the browser, there are also examples such as page caching, which could be done for all unauthenticated requests of some pages.

[damassi]

I’m going to dedicate some time today thinking about this both in terms of solutions as well as tradeoffs.

One thing I think is important to consider is that we should avoid if at all possible offloading our logic into CSS, even if that means incurring a performance hit (which we still need to measure to be sure its even an issue). Having a declarative top-down react-based API thats easy to use and maintain (like the current one) has long term benefits in terms of code maintainability. Would hate for us to throw that away unless the gains are significant. Logically the issue makes sense, but in practice i wonder if there might be other considerations we should evaluate. Gonna put on the thinking cap and see how others do it in the age of React.

(In terms of raw performance, I’ve posted this link before, but for others who haven’t read it I think its an important read: https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578)

[alloy]

I don’t see it as an either or thing. Sure, whatever we’d come up with would probably be at least somewhat different, but that doesn’t mean it needs to be hard to maintain. We should have both and that shouldn’t even be a question.

Consider also that we’re sometimes talking about things such as streaming rendering. There’s no real point to great optimization’s like that if the browser is still going to throw away the results once it has it all.

[alloy]

Ok, here’s a first “wake-up from a dream” stab at a possible declarative way to do the “Pure CSS” option:

    <Responsive>
      {breakpoints => (
        <>
          <breakpoints.xs>Extra-small</breakpoints.xs>
          <breakpoints.lg>Large</breakpoints.lg>
          <breakpoints.else>All other cases</breakpoints.else>
        </>
      )}
    </Responsive>

To be clear, this will generate markup for all these elements, so it should really only be used near the leafs of the tree. However, I think that’s usually where most layout changes will happen that are harder to express with e.g. styled-system’s responsive props, so this might not be a big problem, just requires a bit of education.

A nice thing about a solution like this is that I can see various ways that we could perform further optimisations in the implementation later on. E.g.:

• Decide to only include certain breakpoints in the rendered output for mobile devices where we are 100% sure that they never need the other breakpoints.
• Perform the else breakpoint generation at build time using a babel transform.

[ashfurrow]

Mmm! That's a really clever idea. To confirm I understand, this means with some CSS breakpoints that the browser could decide which component tree to render? I like that it keeps us at the React level of abstraction so we can build those optimizations. I took a look at the recent /order2 work we've done and it feels like the extra DOM elements wouldn't add too much to the payload, but as Chris pointed out, we should benchmark before/after.

[damassi]

@alloy - so far this is looking really nice! let me try it out in reaction to see how things work.

[alloy]

To confirm I understand, this means with some CSS breakpoints that the browser could decide which component tree to render?

@ashfurrow Yeah conceptually that’s a good way to put it, although I’d probably not speak of “components getting rendered” as that could be confusing with react’s rendering of components. The important part is that React only renders once and gives the browser’s rendering engine all the pieces it needs to adjust the layout (without further React intervention).

[ashfurrow]

Right, that makes sense. Two different types of rendering: one is rendering React components, and another is rendering HTML DOM elements based on CSS breakpoints. Right?

[damassi]

I'm doing a little spike to replace all of the responsive components in the /artist route and so far so good; totally painless refactor process. One thought has come up so far which we can test once my spike is done: How will this effect screen readers and other page analysis tools?

[alloy]

@damassi Great point! We absolutely should.

[alloy]

@ashfurrow Exactly 👍 But note that they do exist in the DOM, they are just not shown.

[alloy]

Additionally need to look into things like ensuring images in hidden elements aren’t loaded by the browser. Although that does appear solveable, at a glance https://stackoverflow.com/questions/12158540/does-displaynone-prevent-an-image-from-loading.

[damassi]

Ok y'all, i've got a spike going here:

https://github.com/artsy/reaction/pull/1374

I've noticed a few issues which make me think that this implementation might a) come at an unnecessary complexity cost, or b) be used nicely in conjunction with conditional rendering.

1. There are a lot of instances where refactoring out the old component was straight forward. Instances such as the ArticleItem is a good example:

https://github.com/artsy/reaction/blob/3cedf134e3c2e5ff0d103eca469a51ddc3c1af97/src/Apps/Artist/Routes/Articles/ArtistArticle.tsx#L26-L39

2. A more complex, but similar example is

https://github.com/artsy/reaction/blob/3cedf134e3c2e5ff0d103eca469a51ddc3c1af97/src/Apps/Artist/Components/ArtistHeader.tsx#L39-L58

This outputs two instances of the markup on the page at any given time, which might incur a complexity cost in three or more dimensions: screen readers, page analysis tools and image loading. We would have to be sure to not load images in the background, which is workable but also error prone at scale.

(As an aside related to maintainability and simplicity, note that from a conditional rendering perspective this code could be simplified to just

const AristHeader = xs ? SmallArtistHeader : LargeArtistHeader
return <ArtistHeader {...props} />

)

3. An issue I ran into (which I think is definitely solvable at the API level) is duplicate JSX markup, which would lead to (roughly) identical, duplicate html markup:

https://github.com/artsy/reaction/blob/3cedf134e3c2e5ff0d103eca469a51ddc3c1af97/src/Apps/Artist/Routes/AuctionResults/ArtistAuctionResultItem.tsx#L57-L77

With conditional rendering, its easy to say something along the lines of

if (xs || sm) {
  return <SmallComponent />
} else if (md) {
  return <MediumComponent />
} else {
  return <LargeComponent />
}

Where in the new implementation xs and sm would need to be duplicated. A possible solution would be to establish a way to compose xs and sm together to render a given code path.

4. Another issue I ran into is interweaving breakpoints within a given block of JSX -- also likely solvable:

<Responsive2>
  {breakpoints => {
    return (
      <div>
        Hello 
        <breakpoints.xs>Some small markup</breakpoints.xs>
        <breakpoints.else>Some large markup</breakpoints.else>
        how are you?
        <breakpoints.xs>further down small markup</breakpoints.xs>
        <breakpoints.else>further down large markup</breakpoints.else>
      </div>
    )
  }}

Noticed that for a given file we can only use a breakpoint once, otherwise things break (see artsy logo in the Footer within Artist App, at the bottom of the page, when in xs.)

5. There are lots of examples of our existing conditional rendering being used in ways that doesn't thrash the dom in a way that's problematic. The most straightforward example is our use of the ArtworkGrid, and how many columns we should render.

Current implementation:

<Responsive>
  {({ xs }) => (
    <ArtworkGrid columnNum={xs ? 2 : 3} />
  )
</Responsive>

Another example is more nuanced and (basically) unrelated to the responsive dom. Responsive is used to determine whether or not to expand a hidden list, and this depends on the character count of an artists biography. We determine that here:

https://github.com/damassi/reaction/blob/f002f510f0ed8e3b64829dd1f6e129a6d9ee069e/src/Apps/Artist/Routes/Overview/index.tsx#L138-L143

which digs into the logic here:

https://github.com/damassi/reaction/blob/f002f510f0ed8e3b64829dd1f6e129a6d9ee069e/src/Apps/Artist/Routes/Overview/index.tsx#L66-L69

This kind of behavior is simple and useful, but would be difficult to implement without some kind of matchMedia-like behavior which returns whether or not we're within a given breakpoint dimension.

End thoughts:

If performance really is a measurable issue with the existing Responsive component as it relates to SSR and other concerns, I think this new implementation could be used nicely alongside the existing matchMedia based approach. In most instances where we build out our responsive components in isolation (such as in Pagination, ArtistCard, etc) we're doing simple either / or renders that map nicely to a given breakpoint, which in turn maps well with this new implementation. There are many unknowns in this approach that we'd have to investigate though, such as screen readers and other dom tools that (seem) to expect 1:1 markup-to-view representations for accurate metrics. However, after digging into our current code the picture that appeared was a more nuanced use of Responsive, where it wasn't entire dom trees being rendered depending on a given breakpoint, but rather conditions being evaluated for determining smaller items such as column count, spacing, whether to include a given item here and there, and so on -- subtle details, stuff that works well with the vdom, stuff that doesn't thrash or cause measurable impacts on performance. I believe these kinds of use-cases were greater in number than things seen in bullet point 1.

[damassi]

(cc @ds300 just so its in the feed, hope you're resting up and feeling better)

[alloy]

I can see the existing and new versions being used next to each other.

Regarding column count props, I’m starting to think that they’re kind of an anti-pattern when it comes to responsive design. Even the masonry grid could maybe be expressed with just css/flexbox.

[damassi]

Regarding column count props

In terms of the ArtworkGrid, perhaps so! I think that a css approach (or even better a css-grid approach) would be easy and sufficient. A more representative example of how Responsive is frequently used is here: https://github.com/damassi/reaction/blob/ef6b3ad74a4926cc3d55caa41ad36aa52cd163cf/src/Apps/Order/Routes/Payment/index.tsx#L200

For any given breakpoint change only a tiny subset of the tree is modified.

[alloy]

Can you test if that breaks reconciliation or not?

[damassi]

Sure thing, one sec...

[damassi]

@alloy - I setup a repo for us to experment within that mirrors our real app environment pretty closely and I've added you as a collaborator: https://github.com/damassi/responsive-test

It has the old Responsive on the top and the new on on the bottom. As far as layout stuff goes your approach is indeed totally sturdy at all breakpoints, as expected. While right now (most) of our designs only incorporate two breakpoints -- xs and everything else, which is why the current Responsive gets us by -- we shouldn't assume this to be the rule, and thus this new approach def seems appropriate assuming we can cover edge-cases.

Regarding the conditional render Responsive component and reconciliation, it works as expected:

We should be able to get a good idea of possibilities from this, as well as the issues.

[alloy]

@damassi Ugh, sorry I meant rehydration.

I’ve pushed another version that doesn’t do the magic else because it’s too confusing, doesn’t rely on context (although we could optionally make use of context in the future to ignore large breakpoints from emitting for mobile clients), and deals with some of the other comments in your first spike. https://github.com/artsy/reaction/pull/1379

[zephraph]

I was hesitant on this when it first came up. I wrote up a whole list of reason why we shouldn't try to solve this problem at this time, but I decided I needed to know more first. I've got what I learned below, but it was enough to make me change my mind. It's important but I think it's going to be difficult. I really want to be open about my motivation and how I came to this point though, because I feel like it's important for our growth as a team.

TL;DR

• It's important to talk about performance work in terms of success metrics (something I'm always trying to get better at)
• We should be mindful of other potential performance ramifications to these changes (I think we're largely there)
• Some work is more or less required because react doesn't completely re-render the app on hydration anymore
• I ramble and github doesn't like long responses

Why I was hesitant

The initial driving motivator for this change (as I understood it) was performance. I love performance! Great! Let's do it! Except... this isn't an easy problem. Right now, we have a component that provides a set of boolean flags based on what breakpoint the browser is in. It gives the developer the power to render only what's necessary for that breakpoint. That seems pretty performant (other then, you know... javascript). It's not necessarily ideal when the breakpoint changes because we could potentially be required to re-render most of the view (not to mention that there's javascript running everytime the breakpoint changes), but that's a tradeoff that most users hopefully wouldn't have to pay. Unfortunately it's next to impossible to know a user's exact screensize at server render time. That means, with the current approach we can't guarantee that the client and server code will be the same.

So, here we are. When we start thinking about keeping the markup the same across the client and server, things get a lot hairier. Now, we have to render every component for every breakpoint in one go. Some of them are hidden via css, but javascript for each component to render is being executed. That means for the most part every responsive component we have has to render at least twice (mobile, desktop). Then all of those component variations have to be served in the server side html response. Now our initial html payload is larger, which increases the download and parsing time. That effects real user performance... even if a lot of that isn't actually displayed to the user. Also our server has more work to do. Longer server responses and more work for the server mean increased TTFB and higher server costs. That's not to mention the performance regressions we could see from rendering a lot of hidden images and having to dig in to if there are any SEO ramifications for having all that hidden text. Finally, on a non-performance related note, we'd have to refactor the usage of the Responsive component in at least Reaction, if not also in volt and other places it's used (if we decided to deprecate it).

So that's part of why I was hesitant. We'd have to do some really hard work to get this done, and I see multiple places where it could potentially have a negative impact on performance or come with signficant complexity to attempt to counteract the perf concerns.

Whether it made performance better or worse may not be clear to us though. That's the other part. I've been bitten by changes like this in the past that seem clear cut, but don't end how you'd expect. To explain, let me tell you a story...

A story about a time I failed

I've mentioned before that performance was a focus of mine at my last job. The great thing about that role is that I was put in a position of responsibility and allowed to fail. Performance is an area that I failed in pretty spectacularly, but thankfully it taught me some valuable lessons.

We were coming into a new quarter and my manager told me that he wanted me to lead a performance effort for some of that quarter. I had a few sprints and some talented engineers and I was ready to go! It was going to be 🔥 blazing fast 🔥 when we were done. I did some pre-work to find some really juicey issues too. One was pretty obvious: Our CMS was dumping all sorts of garbage and scripts into every page. Easy win! Except it was hard as heck. We got it done, but it took a lot longer than expected. Still, performance gain. Next thing I focused on was also what I thought was a no-brainer. Bundle splitting. I mean, we had one honking HUGE js bundle for like 6 sites. The work was hard. It was hard, because it ended up affecting a lot of things we didn't intend to. We had to tweak our caching strategy which caused a few production incidents before we got it sorted out. We faffed between payload size and number of requests. Anyway, eventually we got to something we thought was better performing. At least in our devtools it was better performing. I went back to my manager and reported the status of everything. We'd ate up most of our time with these big ticket items, but they were the things that really needed to be fixed. I mean, both of those were obviously wasteful and unoptimized. So, my manager asks what our performance improvement looked like. So I told him about the big things we had accomplished, along with some smaller stuff. He asked me how close we were to being finished (Performance work ends? 😱). I wasn't sure. He asked me what our team determined success looked like. I was like... well, we improved page speed index, start render, and some other things. He asked me again what our team determined success to look like. Then he asked how I'd present it to his boss (the SVP of Digital). Heh, I was a bit nervous then. He asked how the team would prioritize new work. How they'd know 6 months down the line that this work was successful. If every member of the team knew what metrics were important to focus on. How the work we'd done had specifically contributed to our success criteria. What I'd do later to make a business case for further performance work. How I'd express the value of what we were doing.

Heh, it was a surreal thing. It was very humbling and exactly what I needed. We came up with some metrics that were important. In retrospect, the work we did initially didn't move the needle nearly as much as we'd hoped. The team got on the same page, did some in-depth investigation, and came up with some critical suggestions of what to fix to improve performance. It basically boiled down to optimizing the critical rendering path. Still, every thing we did for the rest of that project was strategically selected because we knew it had a measurable impact towards a business objective that the entire team from the QA engineers to the executive staff saw as the standard for what performance meant for our group.

Prioritizing was a lot easier after that. It just came down to two things: How hard is it and what's the potential impact? If we couldn't answer those, we didn't do the work. We succeeded in meeting the goal we set out for. It wasn't all perfect, even after that though. Performance is hard. It's hard, because you have to build it into the culture. It's got to be foremost in how engineering builds something, but it also effects the products in very real ways. Sometimes you have to use a different design because one won't be performant. The business (product, management) have to be bought in to the notion that performance is important. That happens when they know how it affects the business what it costs to fix. One thing is certain though... everyone has to have knowledge and visibility about what the state of your system performance is. Without that, your performance will regress.

Okay, that's my story. It's a bit of a tangent and not 100% relavent to this situation, but hopefully it helps folks provide some insight into how I look at perf problems.

So what does that have to do with this issue?

Ultimately, I'll always hesitate to work on something related to performance if I don't have a general idea of the scope and the potential impact. The scope of this work seems broad to me. It's not just implementing a new responsive component. It's dealing with all the unknowns and the performances tweaks that'll need to come after that worry me. Of course all the refactor work and congnitive overhead of the change isn't to be discounted (nor should it stop us from doing necessary changes now before they get any more difficult). The biggest thing is though... I have no idea what the potential impact of this change is on our performance. Not having to re-render the entire app on page load is going to be more performant than doing the re-render. But what about all the other consequences of this change? Will they be offset or could we actually be in a worse spot? Also, generally what's that re-render costing us anyway? Could we be throwing a lot of engineering effort at something that saves a few milliseconds when doing something more straight forward like image optimization could potentially have a much more significant impact at a lower effort level?

I'm not actually suggesting that's the case. After @alloy and I talked about this originally, I definitely knew it's something we should be thinking more about and was practically kicking myself for not considering it sooner. Even without knowing the performance numbers though, I still think we have to do this work. I think that, because this work is important despite the performance impact (negative or positive) it may have.

Besides performance, why is this important?

That's not exactly the question I set out to answer, but it came out of stumbling upon an answer. I was looking for performance related information, instead I learned a bit more about how React 16 handles hydration.

The first clue I found pretty quickly in the react docs for hydrate.

React expects that the rendered content is identical between the server and the client. It can patch up differences in text content, but you should treat mismatches as bugs and fix them. In development mode, React warns about mismatches during hydration. There are no guarantees that attribute differences will be patched up in case of mismatches.

Well... that doesn't bode well. I thought it just re-rendered the app (did in React 15), but it turns out that it just patches the app. Meaning we could have production code right now that's in a bad state because of html mismatches. Weird bugs like the wrong css classes being applied to an element. Turns out, someone else already experienced this (on their site, not ours). That issue is fascinating, I recommend reading through it. It sounds a lot like where we are currently, though I don't know of us having any instance of that bug yet. Granted, it was also from a pre-release version of React 16 so some things could be straighted out. The general theme though, is that patching different markup is imperfect and could result in issues.

In support of that, here's a snippet from the React 16 release blog

In addition, React 16 is better at hydrating server-rendered HTML once it reaches the client. It no longer requires the initial render to exactly match the result from the server. Instead, it will attempt to reuse as much of the existing DOM as possible. No more checksums! In general, we don’t recommend that you render different content on the client versus the server, but it can be useful in some cases (e.g. timestamps). However, it’s dangerous to have missing nodes on the server render as this might cause sibling nodes to be created with incorrect attributes.

So, we won't always do a complete re-render (alters the perf concerns), but that shit be dangerous yo.

I still have questions about what it costs us in render performance. I know we pay something, I just haven't had the time to dig in to see how much. Still, the above justification makes me think that we need to take a serious look at this while it's still a managable size.

So where does this leave us?

• We can't detect screen sizes on the server so we have pretty limited ability of how we can respond to breakpoints
• Using the responsive component as is could lead to difficult to trace bugs due to partial html patching from hydration (due to #1)
• There are potential performance concerns about going full bore media queries and rendering all components all the time with some hidden

It's a hard problem.

I see three potential options (there could be more!)

1. Keep doing what we're doing. Given the above, we know we can't keep doing the exact same thing. My advice would be to limit our risk as much as possible. Look at analytics data to see what our most common non-mobile screen size is. If the user agent is mobile, render xs else render whatever that common screen size is. Tweak that logic if necessary for important targets (iPad?), but otherwise continue as we are. It'll expose us to risks of bugs for portion of our users and we'll still be at risk if non-reponsive code mismatches even if the breakpoints match. We could use strategies like responsive values from styled-systems (as we should be already) or css-grid to make it a little less painful (could be said about the other steps as well though).
2. Tweak the new responsive component to use boolean logic to re-render mobile and media queries for desktop. Basically changing between xs and sm would trigger a re-render. We can cover a lot of cases for mobile detection, so giving mobile xs by default and trying to keep the markup the same for sm+ would give some performance boons to mobile (isn't rendering all the desktop components, pulling in deskop images, etc) while largely holding to the above suggested strategy for handling desktop. Desktop has a little more performance leeway and generally more stable connections. Handling multiple component re-renders and larger initial html payloads won't have as much of an impact. This increases the complexity of the new responsive component and of course still requires us to refactor a lot. Benchmarking this would be nice.
3. Basically what's suggested by in the thread above. Full bore media queries css display: none.

Regardless of what we do, there are two things we should work on

1. Performance monitoring and benchmarks
2. A way to test if there's a markup mismatch on hydration

Sorry for being so long winded. Hopefully it didn't come across as pendantic. This work is important and the conversation is important. Thanks for keeping it a fresh topic @alloy.

[alloy]

@zephraph Thanks for taking the time to write-up your experience and thoughts! It absolutely does not come across as pedantic, because your intention is sincere and the end-goal is what we all set out to work on; a better performing architecture. I’m absolutely sure that your vigilance when it comes to the more scientific flow of monitoring, benchmarking, and only then optimising will lead to us a better place.

So thanks again for taking the time, please keep being vocal, and never ever kick yourself! (Besides the kick itself being painful, that sounds like a horrible pulled muscle waiting to happen.)

👏👏👏👏

---

With the cautionary “premature optimisation is the root of all evil” out of the way, the reason I keep beating the drum on this now is because besides performance we‘re also working on solidifying the concept of the new front-end stack and that should be build on solid patterns. My thoughts on this specific part of the stack are:

• We shouldn’t ever rely on React’s nuanced attempts to rehydrate mismatching server/client markup, because understanding and working with nuances is hard in general, let alone when it’s very specific “low-level” knowledge such as React’s rehydration details, which is otherwise of little benefit of being fully understood by everybody. My experience is that these are the types of slopes that are just a tad too slippery.

  On the flip-side, having our team better understand nuances of HTML/CSS and browsers is something that they should have to deal with and (learn to) understand better, myself included. i.e. rather than smart JS solutions, we should make sure our team understands how to let browsers do their work to their best ability, which is knowledge that scales beyond any particular framework.

• With a “dumber” approach, such as suggested in this thread above, we can actually make some pretty well established choices (in the future). While we should measure, even just for the pleasure of seeing the difference, some of these don’t technically even need measurement to know that they are proven solutions. E.g.:

  • We can still decide to omit content for certain user-agents where we are sure they will never be used, so mobile doesn’t need to get a bandwidth penalty.
  • We can cache static output on the server, even when omitting content for a few chosen user-agents, so we can get TTFB much lower by e.g. skipping large parts of upstream data fetching.
  • Browsers are good at caching static server responses, so user-agents that get the full markup only need to download that extra data once (until the server decides the cache is out of date) and the server doesn’t need to do much of anything.
  • We can stream server responses without worrying that once fully served React fails rehydration, so that the browser can start rendering parts of the page as soon as data arrives and not see rendered content change seconds later.

Granted, some of these techniques may require us to also become more strategical at deciding when to download what data (e.g. defer certain user specific data to the client), but the point is that we have a foundation on top of which we can make those (well-measured) decisions in the future and they are techniques that our team members should be learning as they aren’t unique to our stack.

[damassi]

Thanks for all the comments everyone! One thing I want to chime in and say is I think concern over React's hydration mechanism is a little bit overly cautious and not based on real world data. I understand that it has been reworked in React 16, but I, and many many many others have been using SSR rendering for many years on all sizes of content, and concerns about the stability of this model never comes up in blogs, posts, whatever. React 16's is considered more flexible and stable. There might be an issue here and there filed away in GH, but until we see a repo showing an obvious false-positive in a way that actually impacts real-world code in a real-world way I don't think we should tip toe around what is heavily-used and heavily unit and integration tested functionality shipped down from the Facebook platform. I would also like to mention that right now the new Artist page has hydration warnings that need to be solved, and things are stable. We do need to be aware, but I don't want this to distract us from the path.

[damassi]

Alright @alloy - the latest API iteration in https://github.com/artsy/reaction/commit/d3dce25d85432f91fca3e66c300e07d753f1a5f9 feels very very good 💯. Only one tiny q about the interactions key but i'm a go at this point.

As far as surfaching the API in the description, I think the tests located here more than suffice. Could you maybe update in a comment or at the top of the PR with the API that was landed on? This seems flexibile enough to build all kinds of things on top of.

[javamonn]

I discussed some of the ongoing work here with @damassi a couple of days ago and initially had some concerns with the current implementation direction stemming from "breaking" the React component model in certain respects, i.e. some components are mounted and rendering but non-visible. Most of my concerns around the performance implications of this are addressed by the second bullet here, namely that this approach opens us up to future optimizations and the recent commit to enable limiting the breakpoint "target" list we render to.

One implementation detail I think we should consider more deeply and measure is around leaving the non-visible components mounted after the initial client side render and rehydration:

even when getting the initial breakpoint right, the performance of changing breakpoints will be worse when a browser has to evaluate JS, recalculate a component tree, diff DOM elements, and update the DOM

This is true, but doesn't take into account that breakpoint changes happen (naively, I don't have real data) a couple times per session at most, while rerenders will occur tens to hundreds of times. The extra non-visible components will continue to occupy resources and it feels like we're optimizing for the wrong path. In context of a point made earlier that our current Responsive usage results in relatively small DOM differences across most breakpoints and "premature optimization...", this could be a follow up item we look into.

Aside from performance implications, mounting all breakpoints and leaving them mounted on the client means our lifecycle methods (particularly componentDidMount / componentWillUnmount) for components under a breakpoint need to be pure functions / side effect free for the most part. I couldn't find a case in reaction where a component conditionally rendered by Responsive would run into issues if replaced by the spiked responsive component, but it's easy to find cases of components with impure cDM lifecyles, e.g. here and here. The reason behind this restriction is easier with an example: in the case of the MinimalCtaBanner, if we fired an analytics event on component mount, we may fire 3 different analytics auth impression events when only a single banner was actually visible to the user.

Relay's createFragmentContainer HOC is another case of this: there are cases where we have relay-connected "container" components deeply nested in view heirarchys, e.g. here. Relay is a relatively harmless example I think though, as data fetched by several components across breakpoints would be de-duplicated (I think?).

Ideally we can restrict usage of Responsive to wrapping only "dumb" view components and ideally lifecycle methods are always pure, but this still creates the need for component authors to have some awareness of a component's usage past the props boundary, its usage context becomes important.

As a sidebar, I've done a good amount of work with react-native-navigation V1 on RN which employs a somewhat similar trick of leaving non-visible component trees mounted as screens are navigated away from. I believe it was done as a performance optimization (re-creating the corresponding native views backing the react components is expensive) but in practice it created various rough edges with component lifecycles not behaving as expected. Redux-connected containers were a major offender: subscriptions to store changes were maintained and forced non-visible screens to rerender, resulting in major responsiveness issues.

To be clear, I don't think these concerns are blocking to the proposed implementation, but I do think some of the current tradeoffs are worth voicing.

[alloy]

This is true, but doesn't take into account that breakpoint changes happen (naively, I don't have real data) a couple times per session at most, while rerenders will occur tens to hundreds of times. The extra non-visible components will continue to occupy resources and it feels like we're optimizing for the wrong path. In context of a point made earlier that our current Responsive usage results in relatively small DOM differences across most breakpoints and "premature optimization...", this could be a follow up item we look into.

Good point 👍

In the future I could see us moving the current dynamic media query matching into the new implementation and have it change the onlyRenderAt value of the context provider, post initial render that is.

Ideally we can restrict usage of Responsive to wrapping only "dumb" view components and ideally lifecycle methods are always pure, but this still creates the need for component authors to have some awareness of a component's usage past the props boundary, its usage context becomes important.

Agreed. We’ll need to expand on the docs, maybe write a README, where we can touch on this. (I’d love to see that being written by others with a fresh set of eyes too, hint hint)

Relay's createFragmentContainer HOC is another case of this: there are cases where we have relay-connected "container" components deeply nested in view heirarchys, e.g. here. Relay is a relatively harmless example I think though, as data fetched by several components across breakpoints would be de-duplicated (I think?).

Yeah Relay queries are de-duped and everything would be fetched regardless of what components end up being shown because these queries are static.

[damassi]

@javamonn these are all really good points.

Ideally we can restrict usage of Responsive to wrapping only "dumb" view components and ideally lifecycle methods are always pure, but this still creates the need for component authors to have some awareness of a component's usage past the props boundary, its usage context becomes important.

but in practice it created various rough edges with component lifecycles not behaving as expected.

I'm a little more than worried that over time this is going to break reacts component model, and how its commonly used. This will cascade down through to lib code and who knows what else, multiplied by number of developers * time, all triggered by a responsive piece of code executing somewhere up the tree.

@alloy does this not concern you? I think at the bare minimum we need to make sure that React works exactly as expected without any gotchas. You mentioned that you spoke with a React core team member recently; would you be able to run this sketch by him or someone else that might be able to provide more feedback on this issue? How will future versions of react handle this? This feels like such a show-stopper when dealing with non-trivial code.

[alloy]

Not really. I read that as people not expecting componentDidMount being invoked before it becoming visible. We’re not doing anything that isn’t perfectly fine React code.

[damassi]

I just tried it here: https://codesandbox.io/s/qq1ll0klm6

[alloy]

When we decide that we don’t want componentDidMount to be called before being visible, I think we can actually do the renderOnlyAt trick based on the browser’s current state during the initial render. Rehydration wouldn’t be perfect, but the outcome of the first render should be visually identical, which to me is the most important part.

[damassi]

I'm down to roll with things and iterate, but side effects are pretty dangerous and lead to weird bugs. It seems like upfront we need to be preventing the lifecycle methods from firing so future devs don't have to worry about tracking down hard to find issues.

Redux-connected containers were a major offender: subscriptions to store changes were maintained and forced non-visible screens to rerender, resulting in major responsiveness issues.

This is also a potential performance bottleneck, and in general this pattern is common across lots of different libs / tools.

[alloy]

Oh darn, yes external libs may be a problem. I guess the only sane way I see to prevent that from triggering is to test if performing the first client side render with onlyRenderAt set to the actual viewport size works as we want (no changing visible content).

[alloy]

Alright, did a quick and dirty spike to verify my idea and it seems to work. These are the steps:

• On the server we render markup for all breakpoints (in the future we’ll limit this for some user-agents).
• The browser receives markup with proper media query styling and will immediately start rendering the expected visual result (for whatever viewport width the browser is at).
• When all JS has loaded, React has booted, and starts rendering our app, we query the browser synchronously for what breakpoint it’s at and then we limit (using renderOnlyAt) the rendered components to the matching breakpoints.
• Additionally, like we currently do with the Responsive component, we register event listeners with the browser to notify the MediaContextProvider when a different breakpoint is matched and then re-render the tree using the new value for renderOnlyAt.

---

I added logs to some components’ componentDidMount lifecycle methods that shouldn’t both be visible at the same time.

1. The server renders all breakpoints and lifecycle methods such as componentDidMount will never be invoked, so all good:

   

2. The browser renders the markup, boots React and the app, which ends up only rendering for the currently matching breakpoint (lg) and thus yields a hydration error as expected, but importantly only mounts 1 component (LargeArtistHeader):

   

3. Resizing the browser window triggers re-renders at new breakpoints, again only mounting 1 component at a time (LargeArtistHeader, then SmallArtistHeader):

   

[javamonn]

Nice! That would address all of the issues I raised. It's definitely a nice feature of the proposed API that we can alter the internal behavior around mounting transparently, i.e. it's an implementation detail that we could be mounting all on the server and mounting only active breakpoint on the client.

This goes back to not coupling ourselves to the internals of React's hydration algorithm, but I'm a little curious as to what state the DOM is left in after the initial hydration. If React is no longer blowing away the DOM on hydration mismatch and attempts to patch instead, naively it seems like the server-rendered other breakpoints would continue to exist in DOM until the next render. Since they're hidden, this seems like a non-issue and would be an improvement over the current situation where we have to render the actual client breakpoint over the server rendered XS.

I thought a bit about whether there'd be a way to make the initial client render match the server render exactly, but can't think of a straightforward way that wouldn't result in componentDidMount being executed on non-visible elements that would just be unmounted on the next render. One way that could work I think but is arguably not good:

Extend Media to maintain a deterministic id per component instance. This id hash is included as a data attribute in the rendered children via cloneElement. On server, all breakpoints are rendered, as implemented. On the initial client render we render the active breakpoint, as implemented. For non-active breakpoints we short circuit the render cycle by using the deterministic id to query the DOM for the server rendered output of the particular media query, and re-render it with dangerouslySetInnerHTML. After the initial render we use the logic as currently implemented: non active breakpoints are not rendered at all. I think this would enable us to match the server rendered output on the client without hydration warnings without mounting non-visible components on the client; React just cares about the vdom output, not how we reached that output.

This approach would carry a bunch of caveats though, some I can think of:

• Children are mounted before parents, so we'd have to shift to using the function-as-child pattern in order to let Media lazily render its children or pull the render output out of the DOM depending on whether it's the initial client render or not.
• Details around maintaining a deterministic id, can we just hash the props at constructor time and each render? Will Media children have to manage making sure the id gets flushed to their DOM root manually, or could we hide this behind some more context and a wrapper component?
• Introducing the need for state, both in the DOM in the form of the Media output data attributes and internally to determine if this is the initial client side render or not.

I'm happy to spike this particular approach to make it more clear but in some respects it seems like a premature optimization. It's not immediately clear to me what we lose or what the potential issues are that hide behind the React hydration warnings, and we're already in a spot where we can match a subset of the server output on the initial client render. Adding to this is the fact that we currently have hydration warnings, and we have been fine (as far as we know at least) to ignore them. If hydration does create issues for us at some point, it seems like we could have a path towards resolving them with minimal API surface changes through something like the above.

The most recent spike implementation gets a 👍from me as-is. it's a clear improvement over our current approach and gives us an on-ramp to iteratively improving our approach with minimal to no API surface churn.

[alloy]

@javamonn I love the way your mind works! I’m all for such experiments to explore what is possible. For the long run, though, I was thinking that we could (in the future) take a look at what a proper React solution could be and propose that to the React team, but that definitely needs some experimentation first.

For now the most important part to me is that the API indeed allows us to change the implementation to allow for these types of changes.

[damassi]

Here are a couple more library examples just so we can compare -- They both use the MatchMedia api and suggest user agent sniffing so as to avoid unnecessary render cycles and the potential for large, duplicate react trees:

• https://github.com/ReactTraining/react-media
• https://github.com/contra/react-responsive

I'd like to raise the question: what if user agent detection is sufficient for Artsy's needs?

[alloy]

AFAIK There’s no good way to know the viewport size when doing SSR, so trying to support multiple breakpoints for all devices will lead to cases where the layout will change on first react render.

User agent sniffing will only (somewhat) tell you the device type, so the suggestion of these libs can only work if you only have differences in layout for eg mobile vs desktop.

Eg https://github.com/ReactTraining/react-media/issues/46#issuecomment-363010520

[damassi]

I understand this is a long-term open problem and that's why there there are so many questions across the web about how to approach SSR. What I'm wondering is if in our use case, being able to use a library to detect common mobile browser types such as express-useragent (or whatever) and piggybacking on isMobile, isTablet and isDesktop might be sufficient for us given our design and our user profiles (meaning, analytics stats), and whether or not we might be running into a "perfect is the enemy of the good" scenereo. The questions that @javamonn raised and of which you iterated on get us back to a similar place with rehydration warnings, but now we're having to work around lifecycles while also maintaining duplicate markup render trees which has the potential to cause larger perf issues down the road if not guarded carefully. I wonder why other libraries haven't walked this approach, and it makes me want to be extra cautious.

[orta]

Not to wade into a massive thread with a naive opinion, but could we send the current browser size in cookie/headers?

[alloy]

A cookie needs to be set by our JS code, so it wouldn’t work for the first request, and even when set it would only be of limited reliability because the window may have changed size since the cookie was set.

[alloy]

FWIW The rehydration warnings are symptoms of potential issues which we understand now and have dealt with the actual issue, namely the user never seeing layout differences between the server-side rendered content and React’s first client-side rendered content. (The only thing React would need to do is remove the elements unnecessary for the current breakpoint, which seems trivial enough for React to deal with in its new way of updating rather than replacing the page.)

I would argue we’re not working around anything in the last iteration. Rather it’s a well thought out solution of which we understand the pros/cons.

[damassi]

Alright 😄 we can't know unless we try, so why not try. (Sorry for being a pain.) If something ends up becoming an issue it would be easy enough to update code to conditionally render without having to refactor.

[alloy]

Yeah the main point of having a declarative API is being able to make changes without further large-scale refactors. So by all means, please try things 👍

[alloy]

(GitHub’s ongoing incident seems to be removing comments, @damassi said something about being able to try things without needing further refactors.)

Yeah the declarative API change is mostly about being able to make changes under the hood without needing further large-scale refactors of the code-base. So by all means, do try things once that API is in 👍

[damassi]

This renderOnlyAt pattern works well! One thing I noticed is we'll need to use <MockBoot> in the tests and set initial breakpoints otherwise it will by default render out two (or however many) items.

[alloy]

Thanks to all of you that participated for the great discussion! 🙌