Partial rendering and hydration of react on the client by oliverlloyd · Pull Request #893 · guardian/dotcom-rendering

oliverlloyd · 2019-10-29T13:11:23Z

What does this change?

This replaces our current approach of fully hydrating the client app with partial hydration of separate react nodes, only where required.

React Islands

Currently we render content on the server and then hydrate the entire app on the client. This technique only hydrates the specific areas of the app that require react, leaving the rest as static html.

The components using this approach are:

Nav (the main pillar menu in the Header)
EditionDropdown
MostViewed
ReaderRevenueLinks
RichLinks
ShareCount

The user experience is that the page will load and whatever content that exited the server render will be present on the page. React is then added to each dynamic part of the page with the following impacts:

`Nav (the main pillar menu in the Header)`

The main pillar menu is immediately present on page load. The More button only starts working after react is loaded

`EditionDropdown`

The dropdown control is visible on the page but does not become active until react is added

`MostViewed`

The entire component is missing on initial load and only appears after react is loaded and the api call to get the content returns

`ReaderRevenueLinks`

This component is missing on initial render and only appears after react is loaded

`RichLinks`

These are hidden on initial load and pop in once react is present and the api call returns

`ShareCount`

This is hidden on initial load and appears once react is present and the api call returns

Next steps

This code tests well and is functional. It is logical that it should be more performant but the improvement is difficult to measure so discussion around an approach for implementation is required.

With this PR in place it would be logical to review the data we pass to the client using window.guardian. This could likely be substantially reduced.

Why?

Performance

Link to supporting Trello card

https://trello.com/c/cA7ymUYg/781-spike-portals

PRBuilds · 2019-10-29T17:08:15Z

PRbuilds results:

💚 AMP validation
amp-report.txt

LightHouse Reporting
1573042149.report.html

--automated message

SiAdcock · 2019-10-30T08:33:22Z

Interesting stuff @oliverlloyd, thanks for the detailed description!

Why not use Portals for Nav?

To complete the circle I have to ask, why not Islands for everything? Islands sounds closer to a progressive enhancement ideal (render as much as possible including placeholders on the server, enhance on the client). With Portals it sounds like we're presenting loading spinners and waiting for everything to be ready before displaying anything useful.

Even if progressive enhancement is not possible for a particular piece of content, isn't it better (i.e. less complex) to have "one way of doing things"? I can't think of any tradeoffs with the Islands approach that necessitates using Portals in some circumstances.

This is my naive reading, please correct me if I have misunderstood 😄

oliverlloyd · 2019-10-30T09:31:20Z

why not Islands for everything?

You're right, replacing Portals with Islands would make no discernible difference to the user experience so having one approach makes sense. This content won't be rendered on the server in any case, not the way things are currently setup, so the initial render would still be empty for these elements.

The advantage to Portals is it's just one instance of react, not one per island. So that means you can share state between different portals. This is nice, but as things stand we don't have a requirement for this. Changes in context (navigation, changing edition, etc.,) trigger page loads so there's no state to manage.

I'll have a look at migrating everything to islands in this PR.

SiAdcock · 2019-10-30T09:50:49Z

Thanks @oliverlloyd, interesting points.

I agree with the YAGNI argument against shared context. However, I wonder if there's a hidden cost in called ReactDom.render() for a whole bunch of Islands, vs calling it once for Portals?

oliverlloyd · 2019-10-30T10:14:59Z

I had the same thought. I guess it depends on how fast

5 x ReactDom.hydrate(React.createElement( ... ), ...)

is vs.

1 x ReactDom.render( ... ) + 5 x ReactDom.createPortal( ... )

…e same component

philmcmahon · 2019-10-31T17:13:22Z

        ...defaultStyles,
        Comment: palette.opinion.faded,
-        AdvertisementFeature: palette.neutral[85],
+        AdvertisementFeature: palette.neutral[86],


is this related?

I think that's where I'm polluting this PR by merging in master, sorry!

philmcmahon · 2019-10-31T17:30:35Z

-        //     },
-        //     true,
-        // );
+        window.guardian.modules.sentry.reportError(error, 'rich-link');


philmcmahon · 2019-10-31T17:34:28Z


 const richLinkContainer = css`
-    width: 8.125rem;
+    ${until.wide} {


hmm it's weird that these changes are showing up here when they were made on my branch/merge

That's me merging in master to fix conflicts. Sorry, I should have waiting for the reviews

philmcmahon

I've looked through this and agree with the motivation behind the PR and the code looks good to me. I would:

Deploy to CODE dotcom:rendering

Test multiple different pages - both with/without ads.
This is a page with a rich link https://m.code.dev-theguardian.com/film/shortcuts/2015/may/18/mad-max-fury-road-crazy-guitar-guy-doof-warrior-turning-it-up-to-11

philmcmahon · 2019-10-31T17:53:35Z

(also looks like cypress tests are failing for some reason in teamcity, I'm happy to help debug this)

nicl

I think I am for this as an improvement over what we have now. But would be great to get numbers for the performance impact before we merge as that is the primary motivation for the change.

oliverlloyd · 2019-10-31T20:58:24Z

There was a talk by Yuzhi Zheng, the React team manager, at ReactConf the other day about a feature they're working on as part of the react core which is very relevant to this PR: Progressive/Selective Hydration. It's only a few minutes long but a really interesting window into how they think our problem should be solved.

Essentially Progressive means it lazy loads islands (renders a section, then hydrates it, then renders the next section, then hydrates it...). And Selective means it will decide the order to render/hydrate islands based on mouse position.

This made me realise we should think about the order that we hydrate islands. For example: it makes a lot of sense to hydrate the Pillar menu and EditionDropdown before the ShareCount or MostViewed.

oliverlloyd · 2019-10-31T21:05:13Z

would be great to get numbers

@nicl Totally agree.

I did try some comparisons using our internal webpagetest server but there was no notable difference in the reported numbers. I suspect wpt isn't giving us a true TTI value.

I also worked with Phil to setup our performance monitoring on Code so we could use that to compare but the granularity isn't great (once a day) and there's a real risk other deploys will impact our control.

I'm interested in the script you demo'd at the client side meeting the other day, maybe we can see if that helps here?

I suspect the key will be gathering stats using a slower device. These are the users who we're doing this work for, people like you and I with modern laptops and phones are unlikely to feel much benefit, esp. not at this stage of the project where we have relatively few interactive elements on the page..

…tcom-rendering into oliver/partial-react

oliverlloyd · 2019-11-01T17:04:45Z

@nicl I used your fast lib to run some tests on my laptop and there's a positive impact. tti Drops from 9.26 to 8.27

| branch        | time  | ps   | tti  | kb     |
|---------------|-------|------|------|--------|
| master        | 14:40 | 0.67 | 9.44 | 583916 |
| master        | 14:45 | 0.66 | 9.09 | 584351 |
| master        | 14:53 | 0.66 | 9.24 | 583990 |
| master        | 15:26 | 0.68 | 9.27 | 583370 |
| master        | 15:31 | 0.68 | 9.24 | 583613 |
|---------------|-------|------|------|--------|
|               |  Avg: | 0.67 | 9.26 | 583848 |
|               |       |      |      |        |
| partial-react | 14:54 | 0.69 | 8.44 | 567966 |
| partial-react | 15:01 | 0.68 | 9.13 | 567973 |
| partial-react | 14:32 | 0.65 | 9.01 | 568239 |
| partial-react | 14:32 | 0.70 | 9.32 | 567376 |
| partial-react | 15:24 | 0.68 | 8.94 | 567325 |
|---------------|-------|------|------|--------|
|               |  Avg: | 0.68 | 8.97 | 567776 |

oliverlloyd · 2019-11-01T21:09:08Z

Another win here is that the react bundle size drops from 76kbs to 60kbs

Full hydration

Partial hydration

🎉

SiAdcock · 2019-11-02T20:41:45Z

Another win here is that the react bundle size drops from 76kbs to 60kbs

Wow! Is the reduction accounted for by components that are rendered on the server only? Or do you think something else is being excluded?

oliverlloyd · 2019-11-02T23:23:07Z

I guess it's a bit like we've implemented code splitting. Before, the code path traversed the whole app, importing everything into the bundle. Now, with the change in this PR, it is limited to certain routes, so many components are never touched. The number of components touched drops from 94 to 54.

These images show the webpack bundle analyser output for the react bundle before and after. You can see that with islands there are substantially fewer files under init.ts, the point where this PR changed the code.

Before (without islands)

(init.ts - 20kb)

After (with islands)

(init.ts - 32kb)

gtrufitt · 2019-11-05T14:38:56Z

With this PR in place it would be logical to review the data we pass to the client using window.guardian

This will be another big advantage. Right now we pass a lot that we don't need, so reducing that and being more specific will be great.

And Selective means it will decide the order to render/hydrate islands based on mouse position.

This is another key IMO - Islands mean we don't have to do any hydration of bottom of article related content until we reach a scroll position (and probably some other components only if use mouse position or even interaction!)

gtrufitt · 2019-11-05T14:40:02Z

Oh and this means we can include useful libraries on the server, without having to use them in the client. In particular, this example: #694

gtrufitt · 2019-11-05T17:49:06Z

+            component: IslandType['component'];
+            // props: any because TS is locking it's checks to the first type
+            // in the IslandProps union, and then failing subsequent items
+            // in out array of islands


Suggested change

// in out array of islands

// in our array of islands

gtrufitt · 2019-11-05T17:50:29Z


            <body>
-                <div id="app">${html}</div>
+                ${html}


…tcom-rendering into oliver/partial-react

Partial rendering and hydration of react on the client

f61b61e

oliverlloyd requested a review from nicl October 29, 2019 13:11

oliverlloyd added 3 commits October 29, 2019 15:59

Fix missing padding for revenue links

35ba0dd

Suppport reader revenue links in footer as well as header

23e2b35

Set geolocation cookie for Percy tests

c76702f

oliverlloyd added 3 commits October 29, 2019 17:44

Move EditionDropdown to islands

c82bb77

Restore set cookie call in Percy scripts

2f994f3

Set correct cookie

9d7a707

oliverlloyd added 5 commits October 30, 2019 11:49

Replace all Portals with Islands

f7584c9

Fix key warning

22dbdc0

Use data attributes in place of ids to allow multiple instances of th…

0bf0bb2

…e same component

Remove unused root element

f145f6e

Merge branch 'master' into oliver/partial-react

2b5e730

philmcmahon reviewed Oct 31, 2019

View reviewed changes

philmcmahon approved these changes Oct 31, 2019

View reviewed changes

nicl suggested changes Oct 31, 2019

View reviewed changes

oliverlloyd and others added 4 commits October 31, 2019 22:45

Fix merge regression

e85482b

Added tests for Dropdown

3f49961

Merge branch 'master' into oliver/partial-react

1cc049c

Added tests for pillar menu

35d4d13

oliverlloyd added 3 commits November 1, 2019 00:45

Merge branch 'oliver/partial-react' of https://github.com/guardian/do…

2fcda62

…tcom-rendering into oliver/partial-react

Added tests for interactivity

941d394

Merge branch 'master' into oliver/partial-react

39f0135

Mock the MostViewed api call to prevent the tabs suddenly dissapearing

d5c2fea

oliverlloyd added 2 commits November 1, 2019 21:54

Remove only limit

0e94eea

Define the order islands are hydrated

5de10cf

Merge branch 'master' into oliver/partial-react

9b05d1f

gtrufitt reviewed Nov 5, 2019

View reviewed changes

Comment thread src/web/server/htmlTemplate.ts

<body>

<div id="app">${html}</div>

${html}

gtrufitt Nov 5, 2019

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

oliverlloyd and others added 5 commits November 6, 2019 11:09

Merge branch 'master' into oliver/partial-react

bf2db15

Merge branch 'master' into oliver/partial-react

00f93e2

Mock fetch call properly

e85dad7

Clearer naming: visitOptions > fetchPolyfill

141253d

Merge branch 'oliver/partial-react' of https://github.com/guardian/do…

e295534

…tcom-rendering into oliver/partial-react

oliverlloyd merged commit 12663c4 into master Nov 6, 2019

oliverlloyd deleted the oliver/partial-react branch November 6, 2019 12:17

oliverlloyd mentioned this pull request Nov 12, 2021

🏝 React Islands #3629

Merged

Conversation

oliverlloyd commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this change?

React Islands

Nav (the main pillar menu in the Header)

EditionDropdown

MostViewed

ReaderRevenueLinks

RichLinks

ShareCount

Next steps

Why?

Link to supporting Trello card

Uh oh!

PRBuilds commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SiAdcock commented Oct 30, 2019

Uh oh!

oliverlloyd commented Oct 30, 2019

Uh oh!

SiAdcock commented Oct 30, 2019

Uh oh!

oliverlloyd commented Oct 30, 2019

Uh oh!

philmcmahon Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

oliverlloyd Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

philmcmahon Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

philmcmahon Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

oliverlloyd Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

philmcmahon left a comment

Choose a reason for hiding this comment

Uh oh!

philmcmahon commented Oct 31, 2019

Uh oh!

nicl left a comment

Choose a reason for hiding this comment

Uh oh!

oliverlloyd commented Oct 31, 2019

Uh oh!

oliverlloyd commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliverlloyd commented Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliverlloyd commented Nov 1, 2019

Full hydration

Partial hydration

Uh oh!

SiAdcock commented Nov 2, 2019

Uh oh!

oliverlloyd commented Nov 2, 2019

Before (without islands)

After (with islands)

Uh oh!

gtrufitt commented Nov 5, 2019

Uh oh!

gtrufitt commented Nov 5, 2019

Uh oh!

gtrufitt Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

gtrufitt Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

oliverlloyd commented Oct 29, 2019 •

edited

Loading

`Nav (the main pillar menu in the Header)`

`EditionDropdown`

`MostViewed`

`ReaderRevenueLinks`

`RichLinks`

`ShareCount`

PRBuilds commented Oct 29, 2019 •

edited

Loading

oliverlloyd commented Oct 31, 2019 •

edited

Loading

oliverlloyd commented Nov 1, 2019 •

edited

Loading