[feature]: Lazy Load/trim unused bytes in main routes #2988

sirugh · 2021-02-03T20:24:32Z

Description

Our main client and vendor bundles are pretty large. There are savings (130k split from this PR alone!) to be had by splitting code that is unnecessary for the first render. That would include things like modals, drawers, drop downs, etc. The majority of changes in this PR are just adding React.lazy to components that aren't necessary on first render, and wrapping their implementations in <Suspense>.

Note: Much of the remaining unused bytes are from dependencies that we must load at start, such as Apollo, but for whatever reason do not tree-shake. Other bytes are from our implementation of ContextProviders which expose a lot of unnecessary actions in the main bundle. Further work is necessary to trim out that deprecated code. Definitely more work than is in the scope of this specific ask.

Testing these changes proved more difficult than I would have liked. Kent Dodds has a way around testing Suspense/Lazy but we don't have react-testing-library at our disposal right now. The current approach (mocking React.lazy) is a stop-gap.

Related Issue

Closes PWA-1109.

Acceptance

Verification Stakeholders

Specification

Verification Steps

Go to the home page/route.
"Clear site data" in dev tools.
Run the coverage tool.
Compare numbers to those on the same route/page on develop.pwa-venia.com. They should be better.
Repeat step 1-4 for all of the routes including: cms, category, product page, cart, checkout and my account.

Screenshots / Screen Captures (if appropriate)

Develop Category Page

"Improved" Category Page

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.

Signed-off-by: sirugh <rugh@adobe.com>

sirugh · 2021-02-03T20:25:56Z

docker-compose.yml

@@ -33,7 +33,6 @@ services:
      - ./packages/venia-concept/static:/usr/src/app/packages/venia-concept/static:rw
      - ./packages/venia-ui/.storybook:/usr/src/app/packages/venia-ui/.storybook:rw
      - ./packages/venia-ui/lib:/usr/src/app/packages/venia-ui/lib:rw
-      - ./packages/venia-ui/templates:/usr/src/app/packages/venia-ui/templates:rw


For starters, I might have scope creeped this a bit but as far as I can tell we no longer use the templates, opting instead to use venia-concept/template.html. @zetlen @revanth0212 can ya'll confirm this?

You are right. We got rid of mustache templates in favor of Webpack's HTML Plugin which utilized template.html to render the main HTML file.

PWAStudioBot · 2021-02-03T20:35:48Z

	Messages
📖	Associated JIRA tickets: PWA-1109.
📖	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 cdd2b0e

jimbo

Makes sense to me. Good savings. 👍

Signed-off-by: sirugh <rugh@adobe.com>

sirugh · 2021-02-04T21:31:48Z

packages/venia-ui/lib/components/App/__tests__/__snapshots__/app.spec.js.snap

+  <() => {
+  /* istanbul ignore next */
+  cov_101e9trynu().f[0]++;
+  cov_101e9trynu().s[1]++;


I don't know why code coverage started showing up in some of the snapshots but not others. It has something to do with the jest mock. When the mock is local to the component being tested the coverage comments do not appear.

ok it looks like this was happening because I was running with some jest filter like "pattern" or "failed test" which caused the reporter to attach comments for that run. When I run the entire suite, they are removed.

...b/components/CartPage/PriceAdjustments/__tests__/__snapshots__/priceAdjustments.spec.js.snap

revanth0212 · 2021-02-05T14:22:26Z

packages/venia-ui/lib/components/App/__tests__/app.spec.js

@@ -160,7 +159,8 @@ test('renders a full page with onlineIndicator and routes', () => {
    };
    const { root } = createTestInstance(<App {...appProps} />);

-    getAndConfirmProps(root, Navigation);
+    // TODO: Figure out how to mock the React.lazy call to export the component


Hey dude, is this still valid given you have added a react mock in venia-ui mocks?

Yes. There is no Navigation component, just the stringified version of the React.lazy callback.

Signed-off-by: sirugh <rugh@adobe.com>

revanth0212

Nice work buddy. Couple of minor questions but mostly it is ready for QA.

...venia-ui/lib/components/CheckoutPage/PaymentInformation/__tests__/paymentInformation.spec.js

revanth0212 · 2021-02-05T14:52:18Z

packages/venia-ui/upward.yml

-    resolver: directory
-    directory:
-        resolver: inline
-        inline: './templates'


Hey, dude thanks for taking care of this. But I would suggest checking with the community if someone is using them or if we are missing something?

These have not been used in PWA Studio since September 2019, after we swapped to the webpack template.html file. Anyone who was on the version from before then and updates since then, will have had to do this upgrade. This is just removing the files that aren't hooked up to anything.

Signed-off-by: sirugh <rugh@adobe.com>

dpatil-magento · 2021-02-18T22:15:49Z

@sirugh Lighthouse score on desktop looks good but mobile still shows unused js warning, pls check -

sirugh · 2021-02-19T14:19:40Z

@dpatil-magento, are you saying that desktop and mobile have different lighthouse scores? That's odd. Either way, in the PR description I did try to clarify why there is still unused js:

Note: Much of the remaining unused bytes are from dependencies that we must load at start, such as Apollo, but for whatever reason do not tree-shake. Other bytes are from our implementation of ContextProviders which expose a lot of unnecessary actions in the main bundle. Further work is necessary to trim out that deprecated code. Definitely more work than is in the scope of this specific ask.

fooman · 2021-02-19T22:59:57Z

Mobile would nearly always be lower as you have the same time targets with less bandwidth and less cpu. Still anything improving desktop should also improve mobile. Would love to see this merged with further efforts coming in additional prs.

sirugh · 2021-02-20T18:36:30Z

@fooman yes, in reality, but does lighthouse throttle automatically? If so that would explain it :)

fooman · 2021-02-20T21:06:54Z

Depends on how you run it. But for example via Google Chrome

the result will show what throttling was applied

or via webpagetest.org

fooman · 2021-02-20T21:21:54Z

So looking at the results via https://webpagetest.org/ and comparing it to PR3000 this PR looks to be an improvement

https://webpagetest.org/result/210220_DiG7_13e1bbb50b7351e69bc60f1c6a16853b/
vs
https://webpagetest.org/result/210220_DiS0_f09ec649ab7eee75b1cabd9b573fe1ff/

First Contentful Paint
4.862s
vs
6.947s

Largest Contentful Paint
10.876s
vs
12.758s

Lighthouse score
22
vs
8

As always these are to be taken with a grain of salt as when you compare the time to first byte there is big difference too which I believe is the time to serve index.html (which should be very much the same between these two PRs):
1.193s
vs
2.848s

fooman · 2021-02-21T19:54:40Z

I reran the above

PR 3000
https://webpagetest.org/result/210221_DiQP_73de239e4eb7cee73cc4816eebaab02e/
First Byte
1.062s

First Contentful Paint
4.982s

Largest Contentful Paint
11.801s

Lighthouse
12

showing the variability of results (I believe a lot may be driven by how the PRs are hosted) and this time not a clear cut improvement. (lighthouse score up but Largest Contentful Paint 6 seconds slower!)

This PR
https://webpagetest.org/result/210221_Di9R_5a284d8d6785c73d84ac613a235c0bd9/

First Byte
0.824s

First Contentful Paint
6.034s

Largest Contentful Paint
17.380s

Lighthouse
17

To run any tests like the above having a hosted version on a production like environment will probably give the best indication how much of an improvement there is.

dpatil-magento · 2021-02-23T19:13:39Z

@sirugh @fooman WPT results are might depend on backend performance also. Here I compared 3 instances and this PR instance looks better. But @sirugh any reason js byte size is more on this pr https://www.webpagetest.org/video/compare.php?tests=210223_DiAX_2d42c6d682b41cde18176b657f3ae906,210223_DiXG_8ffb29c506015a3c670612e84235d0d2,210223_DiTF_b9a81f142cab77a940def48ca0fdfaf9 ?

sirugh · 2021-02-23T19:21:57Z

@sirugh any reason js byte size is more on this pr

@dpatil-magento without any investigation, my gut reaction is that this is because we are now creating more chunks which means more boilerplate JS.

fooman · 2021-02-23T19:28:14Z

With more chunks the JS minification will not be able to create as many savings.

And indeed that's probably the more succinct way to say it @dpatil-magento with fluctuating backend performance the frontend can be hard to isolate.

…-load-unused-bytes

Signed-off-by: sirugh <rugh@adobe.com>

dpatil-magento · 2021-02-24T18:42:04Z

Given its a balanced approach going ahead with this. QA Approved

sirugh added 2 commits February 2, 2021 16:32

Remove unused templates directory

589fa24

Signed-off-by: sirugh <rugh@adobe.com>

Lazy load components and modals that arent necessary on first render

de85854

Signed-off-by: sirugh <rugh@adobe.com>

sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Feb 3, 2021

m2-community-project bot added Progress: ready for review labels Feb 3, 2021

sirugh commented Feb 3, 2021

View reviewed changes

m2-community-project bot assigned revanth0212 Feb 3, 2021

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

PWAStudioBot added pkg:venia-concept pkg:venia-ui labels Feb 3, 2021

sirugh changed the title ~~[feature]: Lazy Load unused bytes~~ [feature]: Lazy Load/trim unused bytes in main routes Feb 3, 2021

jimbo reviewed Feb 3, 2021

View reviewed changes

m2-community-project bot assigned jimbo Feb 3, 2021

Add react-lazy mock so we can keep using snapshot testing to assert

230398b

Signed-off-by: sirugh <rugh@adobe.com>

sirugh commented Feb 4, 2021

View reviewed changes

...b/components/CartPage/PriceAdjustments/__tests__/__snapshots__/priceAdjustments.spec.js.snap Outdated Show resolved Hide resolved

Merge branch 'develop' into rugh/pwa-1109-lazy-load-unused-bytes

8301136

PWAStudioBot added the pkg:peregrine label Feb 4, 2021

revanth0212 reviewed Feb 5, 2021

View reviewed changes

remove instanbul...

feadfb2

Signed-off-by: sirugh <rugh@adobe.com>

revanth0212 previously approved these changes Feb 5, 2021

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Feb 5, 2021

add comment

8e671c6

Signed-off-by: sirugh <rugh@adobe.com>

sirugh dismissed revanth0212’s stale review via 8e671c6 February 5, 2021 15:14

revanth0212 previously approved these changes Feb 18, 2021

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Feb 18, 2021

Merge branch 'develop' into rugh/pwa-1109-lazy-load-unused-bytes

009da4d

dpatil-magento dismissed revanth0212’s stale review via 009da4d February 18, 2021 18:53

dpatil-magento force-pushed the rugh/pwa-1109-lazy-load-unused-bytes branch from 82cc606 to 009da4d Compare February 18, 2021 18:53

Merge branch 'develop' into rugh/pwa-1109-lazy-load-unused-bytes

7fc44bc

Merge remote-tracking branch 'origin/develop' into rugh/pwa-1109-lazy…

43dd4df

…-load-unused-bytes

revanth0212 previously approved these changes Feb 23, 2021

View reviewed changes

fix snap

dc5e8d1

Signed-off-by: sirugh <rugh@adobe.com>

sirugh dismissed revanth0212’s stale review via dc5e8d1 February 24, 2021 17:21

Merge branch 'develop' into rugh/pwa-1109-lazy-load-unused-bytes

cdd2b0e

revanth0212 approved these changes Feb 24, 2021

View reviewed changes

dpatil-magento merged commit 9c4e558 into develop Feb 24, 2021

dpatil-magento deleted the rugh/pwa-1109-lazy-load-unused-bytes branch February 24, 2021 18:43

m2-community-project bot added Progress: done and removed Progress: approved labels Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature]: Lazy Load/trim unused bytes in main routes #2988

[feature]: Lazy Load/trim unused bytes in main routes #2988

sirugh commented Feb 3, 2021 •

edited

Loading

sirugh Feb 3, 2021

revanth0212 Feb 3, 2021

PWAStudioBot commented Feb 3, 2021 •

edited

Loading

jimbo left a comment

sirugh Feb 4, 2021

sirugh Feb 5, 2021

revanth0212 Feb 5, 2021

sirugh Feb 5, 2021

revanth0212 left a comment

revanth0212 Feb 5, 2021

sirugh Feb 5, 2021

dpatil-magento commented Feb 18, 2021

sirugh commented Feb 19, 2021

fooman commented Feb 19, 2021

sirugh commented Feb 20, 2021

fooman commented Feb 20, 2021

fooman commented Feb 20, 2021

fooman commented Feb 21, 2021 •

edited

Loading

dpatil-magento commented Feb 23, 2021

sirugh commented Feb 23, 2021

fooman commented Feb 23, 2021 •

edited

Loading

dpatil-magento commented Feb 24, 2021

[feature]: Lazy Load/trim unused bytes in main routes #2988

[feature]: Lazy Load/trim unused bytes in main routes #2988

Conversation

sirugh commented Feb 3, 2021 • edited Loading

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PWAStudioBot commented Feb 3, 2021 • edited Loading

jimbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revanth0212 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpatil-magento commented Feb 18, 2021

sirugh commented Feb 19, 2021

fooman commented Feb 19, 2021

sirugh commented Feb 20, 2021

fooman commented Feb 20, 2021

fooman commented Feb 20, 2021

fooman commented Feb 21, 2021 • edited Loading

dpatil-magento commented Feb 23, 2021

sirugh commented Feb 23, 2021

fooman commented Feb 23, 2021 • edited Loading

dpatil-magento commented Feb 24, 2021

sirugh commented Feb 3, 2021 •

edited

Loading

PWAStudioBot commented Feb 3, 2021 •

edited

Loading

fooman commented Feb 21, 2021 •

edited

Loading

fooman commented Feb 23, 2021 •

edited

Loading