[PWA-1110] Investigate render/blocking in Venia (Performance) #2952

tjwiebell · 2021-01-19T22:48:18Z

Description

As a developer, I want review and address issues with blockimg/response time (LCP/TBT) to achieve higher Google Lighthouse v6 performance scores as they relate to the shopping experience. This could affect the core code, Webpack and other areas. Analysis from PWA-775 can be found here: https://wiki.corp.magento.com/display/COREENG/Lighthouse+Performance+Audit+-+v8

In-scope

Deep analysis and review of blocking/response using DevTools and other resources. Recommend and remediate issues where needed.

Requirements

Investigate render/blocking with Venia in current state
Identify and remediate (fix) issues as appropriate; possible spike or docs
Testing and validation with Google Lighthouse

Implementation Note

This PR covers a single render blocking point at the very top of the tree added with i18n support. Instead of waiting on the network, the request flow will now cascade in this order:

Immediately render before messages are even loaded. This means defaultMessage props will be rendered at this point.
While network request for current store locale is in flight, use DEFAULT_LOCALE global that is queried for at build time and pre-load that language pack.
Once network request comes back, load language pack for that locale

For the happy path of an {{en_US}} store, this means we now immediately render at step 1, and even when step 2/3 completes, nothing in the application needs to re-render. For alternative paths that return a different locale, the app immediately renders with placeholders, and then re-renders with correctly translated strings at step 3. The flash of english strings happens in the application today, this just renders everything much faster.

Followup tickets have been added to the backlog to address the already existing flash of default locale strings; a simple solution being, always rely on build time storeConfig and preload messages while the network request is in flight. If the build time config becomes stale, the app would still recover, while still rendering immediately. We've also discussed rendering actual placeholders (grey boxes like skeleton rendering) instead of defaultMessages.

Related Issue

[PWA-1110] Investigate render/blocking in Venia (Performance)

Acceptance

Verification Stakeholders

Specification

Verification Steps

Go to category page and clear all cache data
Run Lighthouse Performance audit
In the performance trace, verify that the getLocale graphql request is no longer render blocking (requests don't wait to fire until it returns)
Not required, but you can also compare Performance metrics with develop.pwa-venia.com. Network latency can cause these to be difficult to compare, but TTI and TBT should be lower, and the performance score should gain a few points.
Verify translation still works by switching to French store

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.
I have added translations for new strings, if necessary.
I have updated the documentation accordingly, if necessary.

- No longer block while in loading state

PWAStudioBot · 2021-01-19T22:58:32Z

	Messages
📖	Associated JIRA tickets: PWA-775.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against aca1523

fooman · 2021-01-20T00:41:27Z

Suggest to include an update to workbox. It includes an update to how manifests are loaded GoogleChrome/workbox#2528 which to my reading made the trace more in line with what I expected to load early (in some traces I for example had root components load that were not on the current page before actual images on the page were loaded).

tjwiebell · 2021-01-21T22:11:05Z

Suggest to include an update to workbox 5. It includes an update to how manifests are loaded GoogleChrome/workbox#2528 which to my reading made the trace more in line with what I expected to load early (in some traces I for example had root components load that were not on the current page before actual images on the page were loaded).

@fooman
I was just reviewing profiling results with the team, and we were all baffled why the service worker would do a flood of precache requests before the app has even finished rendering; I am very interested in this upgrade. It looks like we'd have to upgrade webpack as well, but I'll do some digging and if this won't have a huge impact I'll do it. Thanks for the heads up 👍

fooman · 2021-01-21T22:16:53Z

indeed I am on "webpack": "~4.40.0", but don't recollect that being an issue either.

sirugh · 2021-01-22T15:53:32Z

packages/venia-ui/lib/components/App/localeProvider.js

@@ -61,13 +64,13 @@ const LocaleProvider = props => {
        }
    };

-    if (loading) return <LoadingIndicator global={true} />;
+    if (!messages) return <LoadingIndicator global={true} />;


I wonder which is better:

a) Showing a loading indicator until fetchLocale resolves or
b) Not showing a loading indicator, ever, opting to have a potential "flash" when the translated strings come back from fetchLocale.

It would be really cool if we could read defaultMessage strings from some bundled file that agencies could customize depending on their own default language...

That's comparing develop.pwa-venia.com to this PR, with this aiming to remove the render blocking of the loading indicator. Rendering the loading indicator means the context isn't getting rendered, including its children (the rest of the app).

The second part you outlined is exactly what this change does; while Apollo is still waiting on its response, we default to english and render the app immediately. If the response comes back and it's the same as the default, nothing re-renders.

I think the ideal solution here is to apply this logic to all stores; we can precache storeConfig at build time about all stores and immediately load the {StoreConfig.locale} language pack. If we don't want to require rebuilds when config changes, we can leave the query in place that will true up with the network to validate the precached config. I think we've been hinting at needing this solution for awhile, and would be another application to prevent unnecessary re-renders.

@tjwiebell a setting to have storeConfig pre-loaded would be great.

- Add tests for change

sirugh

Short and sweet. 👍

dpatil-magento · 2021-01-28T18:36:45Z

QA Approved.

- Provide default value to load messages

faba313

- No longer block while in loading state

m2-community-project bot added the Progress: ready for review label Jan 19, 2021

PWAStudioBot added the pkg:venia-ui label Jan 19, 2021

sirugh reviewed Jan 22, 2021

View reviewed changes

m2-community-project bot assigned sirugh Jan 22, 2021

m2-community-project bot added Progress: review and removed Progress: ready for review labels Jan 22, 2021

m2-community-project bot assigned fooman Jan 23, 2021

fooman removed their assignment Jan 23, 2021

- Inject default locale as a global and remove render block

0438925

- Add tests for change

tjwiebell added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jan 25, 2021

tjwiebell marked this pull request as ready for review January 25, 2021 20:29

PWAStudioBot added the pkg:venia-concept label Jan 25, 2021

sirugh approved these changes Jan 26, 2021

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Jan 26, 2021

dpatil-magento added 2 commits January 27, 2021 09:48

Merge branch 'develop' into tommy/lighthouse-render-time

8557107

Merge branch 'develop' into tommy/lighthouse-render-time

aca1523

dpatil-magento merged commit d39d7b6 into develop Jan 28, 2021

dpatil-magento deleted the tommy/lighthouse-render-time branch January 28, 2021 18:38

m2-community-project bot removed the Progress: approved label Jan 28, 2021

m2-community-project bot added the Progress: done label Jan 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PWA-1110] Investigate render/blocking in Venia (Performance) #2952

[PWA-1110] Investigate render/blocking in Venia (Performance) #2952

tjwiebell commented Jan 19, 2021 •

edited

Loading

PWAStudioBot commented Jan 19, 2021 •

edited

Loading

fooman commented Jan 20, 2021 •

edited

Loading

tjwiebell commented Jan 21, 2021 •

edited

Loading

fooman commented Jan 21, 2021

sirugh Jan 22, 2021 •

edited

Loading

tjwiebell Jan 22, 2021

fooman Jan 23, 2021

sirugh left a comment

dpatil-magento commented Jan 28, 2021

[PWA-1110] Investigate render/blocking in Venia (Performance) #2952

[PWA-1110] Investigate render/blocking in Venia (Performance) #2952

Conversation

tjwiebell commented Jan 19, 2021 • edited Loading

Description

Implementation Note

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jan 19, 2021 • edited Loading

fooman commented Jan 20, 2021 • edited Loading

tjwiebell commented Jan 21, 2021 • edited Loading

fooman commented Jan 21, 2021

sirugh Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

tjwiebell Jan 22, 2021

Choose a reason for hiding this comment

fooman Jan 23, 2021

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

dpatil-magento commented Jan 28, 2021

tjwiebell commented Jan 19, 2021 •

edited

Loading

PWAStudioBot commented Jan 19, 2021 •

edited

Loading

fooman commented Jan 20, 2021 •

edited

Loading

tjwiebell commented Jan 21, 2021 •

edited

Loading

sirugh Jan 22, 2021 •

edited

Loading