Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 19, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d530a7
fix avatar server client render mismatch
robin-drexler Feb 16, 2020
80598cb
update Unreleased.md
robin-drexler Feb 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions UNRELEASED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
- Fixed container's width on `Modal` ([#2692](https://github.com/Shopify/polaris-react/pull/2692))
- Fixed the position property for the backdrop on `Select` from being overwritten by the focus ring ([#2748](https://github.com/Shopify/polaris-react/pull/2748))
- Fixed `ResourceItem` `Actions` visibility on mouse out ([#2742](https://github.com/Shopify/polaris-react/pull/2742))
- Fixed initial server / client render mismatch in `Avatar` ([#2751](https://github.com/Shopify/polaris-react/pull/2751))

### Documentation

Expand Down
6 changes: 4 additions & 2 deletions src/components/Avatar/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ import React, {useState, useCallback, useEffect} from 'react';
import {classNames, variationName} from '../../utilities/css';
import {useFeatures} from '../../utilities/features';
import {useI18n} from '../../utilities/i18n';
import {isServer} from '../../utilities/target';
import {useIsAfterInitialMount} from '../../utilities/use-is-after-initial-mount';

import {Image} from '../Image';

import styles from './Avatar.scss';
Expand Down Expand Up @@ -46,6 +47,7 @@ export function Avatar({
}: AvatarProps) {
const i18n = useI18n();
const {newDesignLanguage} = useFeatures();
const isAfterInitialMount = useIsAfterInitialMount();

function styleClass(name?: string) {
const finalStyleClasses = newDesignLanguage
Expand Down Expand Up @@ -100,7 +102,7 @@ export function Avatar({
);

const imageMarkUp =
source && !isServer && status !== Status.Errored ? (
source && isAfterInitialMount && status !== Status.Errored ? (
<Image
className={styles.Image}
source={source}
Expand Down
12 changes: 7 additions & 5 deletions src/components/Avatar/tests/Avatar-ssr.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ import React from 'react';
import {mountWithAppProvider} from 'test-utilities/legacy';
import {Avatar, Image} from 'components';

jest.mock('../../../utilities/target', () => ({
get isServer() {
return true;
},
}));
jest.mock('../../../utilities/use-is-after-initial-mount', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
}));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :shipit: :)

return {
useIsAfterInitialMount: () => {
return false;
},
};
});

describe('<Avatar /> Server-side only', () => {
it('does not render an Image', () => {
Expand Down