[Avatar] fix initial server / client render mismatch #2751

robin-drexler · 2020-02-16T03:19:08Z

WHY are these changes introduced?

In every project that uses/renders the Avatar component on server and client, we get a React warning about the initial rendering not being equal.

While mismatches in server-rendered in client-rendered results usually don't seem to be as much of an issue anymore as of react 16, they can be if nodes are missing in the server-rendered output, which I think is precisely the case here.

Also, 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.
However, it's dangerous to have missing nodes on the server render as this might cause sibling nodes to be created with incorrect attributes.

https://reactjs.org/blog/2017/09/26/react-v16.0.html
Example issue: facebook/react#10591

There's another issue. Unfortunately, it seems like React only ever warns for the first mismatch. In some projects, (almost) all pages contain the Avatar component, and since it always produces an error, we might have been missing others.

WHAT is this pull request doing?

Instead of not rendering the image on the server and rendering it on the client, we now wait for the component to be mounted to render the image. So it won't be part of the server render, but also not part of the initial client side render anymore.

This does cause an addional rerender, but I'd assume that's ok(?)
The change should have no effect on the UX/behavior of the component.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import React from 'react';
import {Page, Avatar} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <Avatar source="https://example.com/does-not-exist" initials="WE" />
      <Avatar source="http://placekitten.com/200/200" initials="WE" />
    </Page>
  );
}

First avatar will fail to load the image and show the initials.
Second avatar loads and shows image.

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

github-actions · 2020-02-17T15:23:01Z

🟡 This pull request modifies 3 files and might impact 12 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:

All files potentially affected (total: 12)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Avatar/Avatar.tsx (total: 12)

Files potentially affected (total: 12)

🧩 src/components/Avatar/tests/Avatar-ssr.test.tsx (total: 0)

Files potentially affected (total: 0)

robin-drexler · 2020-02-18T21:45:27Z

I will fix the conflicts in UNRELEASED.md after a review. Otherwise I'll have to do that multiple times a day. :D

BPScott

I've not tried this with an SSR rendering but it sounds like it'll do the trick.

I'd be interested to hear some thoughts from @dleroux and @tmlayton as they looked at this the first time around in #712. Entertainingly it sounds like this "trigger a second render" approach was the original solution but then Staff Engineers happened and it turns out you shouldn't do "isServer" because of this hydration mismatching :D (For fairness my initial thought of "do we need this, could we just render the Image on the server" was also neatly shot-down in that original PR)

BPScott · 2020-02-19T02:42:04Z

src/components/Avatar/tests/Avatar-ssr.test.tsx

-    return true;
-  },
-}));
+jest.mock('../../../utilities/use-is-after-initial-mount', () => {


It doesn't feel like this test adds much value anymore, mocking out individual hooks feels like a weird smell. Do we care that much about initial renderings?

Strictly speaking this isn't even testing SSR rendering - it's a bad proxy of it by mocking out a single hook. I wonder if ideally react-testing should provide some version of mountOnServer that renders a component using react-dom/server (like the hack described in testing-library/react-testing-library#561), but that feels like a meaty problem for another day.

On a very petty note, we can save the explicit return:

jest.mock('../../../utilities/use-is-after-initial-mount', () => ({ useIsAfterInitialMount: () => false, }));

To me it provides the same value as before. Earlier we were pretending to be on a server environment by mocking a constant now we simulate that by mocking a hook. Even though I agree earlier it was more explicit. I also agree that a renderOnServer function that doesn't mount would be pretty great for these cases. Do you want me to delete the test?

As for the petty note: I generally try to avoid to use single line arrow functions for two reasons. The () in ({}) easy to forget, which leads to the function not returning anything. And secondly, if you want to add a second line, it's quite tedious to make that happen and I don't see any advantage of using the single line notation in the first place.
That being said, it's personal preference and I'd be open to change it.

If you think it's still got some value let's keep it :)

Same goes for the petty note bits - you've got a solid rational, and it's personal pref vs idle musings.

Keep it all as it is and :)

robin-drexler marked this pull request as ready for review February 16, 2020 03:25

robin-drexler requested a review from BPScott February 17, 2020 13:24

robin-drexler force-pushed the fix-avatar-ssr-csr-mismatch branch from 03b725a to a357d64 Compare February 17, 2020 15:20

BPScott approved these changes Feb 19, 2020

View reviewed changes

robin-drexler requested a review from dleroux February 19, 2020 12:53

dleroux approved these changes Feb 19, 2020

View reviewed changes

robin-drexler added 2 commits February 19, 2020 09:58

fix avatar server client render mismatch

4d530a7

update Unreleased.md

80598cb

robin-drexler force-pushed the fix-avatar-ssr-csr-mismatch branch from a357d64 to 80598cb Compare February 19, 2020 14:59

robin-drexler merged commit cfec523 into master Feb 19, 2020

robin-drexler deleted the fix-avatar-ssr-csr-mismatch branch February 19, 2020 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Avatar] fix initial server / client render mismatch #2751

[Avatar] fix initial server / client render mismatch #2751

Uh oh!

robin-drexler commented Feb 16, 2020 •

edited

Loading

Uh oh!

github-actions bot commented Feb 17, 2020 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 12)

Files potentially affected (total: 0)

Uh oh!

robin-drexler commented Feb 18, 2020

Uh oh!

BPScott left a comment

Uh oh!

BPScott Feb 19, 2020

Uh oh!

robin-drexler Feb 19, 2020

Uh oh!

BPScott Feb 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Avatar] fix initial server / client render mismatch #2751

[Avatar] fix initial server / client render mismatch #2751

Uh oh!

Conversation

robin-drexler commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files potentially affected (total: 0)

Files potentially affected (total: 12)

Files potentially affected (total: 0)

Uh oh!

robin-drexler commented Feb 18, 2020

Uh oh!

BPScott left a comment

Choose a reason for hiding this comment

Uh oh!

BPScott Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

robin-drexler Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

BPScott Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robin-drexler commented Feb 16, 2020 •

edited

Loading

github-actions bot commented Feb 17, 2020 •

edited

Loading