Skip to content
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.

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

Use lazy loading for routes #5914

Merged
merged 1 commit into from Jun 22, 2023

Conversation

tsmock
Copy link
Contributor

@tsmock tsmock commented Jun 13, 2023

This reduces the amount of date that must be transmitted in order for the initial page (whatever it is) to load.

For the purposes of this analysis, I am using statoscope.tech with download speeds equivalent to fast 3g (1.4 MBit/s).

Total Size Initial Size Initial download time Packages Size Build Time Modules Duplicate Modules Chunks Assets Packages Package Copies
de65ce1 6.39 mb 17.33 mb 101.2s 23.49 mb 103.8s 4000 13 1597 3244 272 5
e11bc43 3.70 mb 7.62mb 44.5s 23.48 mb 237.6s 4033 13 1631 3335 272 5
e11bc43 with #5908 3.64 mb 3.64 mb 21.3s 23.15 mb 37.6s 4020 0 1638 1689 265 6

Note: Build time seems to be highly variable between runs; de65ce1 took 75.2s and e11bc43 took 76.9s in one run.

Initial chunks:

Commit File Parsed Size File Size Download Time
de65ce1 main 3.29 mb 3.24 mb 18.9s
e11bc43 main 2.55 mb 2.79 mb 16.3s
de65ce1 runtime-main 0 51.05kb 291ms
e11bc43 runtime-main 0 52.99 kb 302ms
de65ce1 vendor packages 6.33 mb 3.09 mb 18.1s
e11bc43 vendor packages 2.23 mb 881.48 kb 5.0s

The top 3 dependencies in de65ce1 vendor packages are refractor, mapbox-gl, and date-fns (~3.5 mb). I believe this size is without minification.
The top 3 dependencies in e11bc43 vendor packages are @sentry, react-select, and @formatjs (~1.2 mb). Those are also in de65ce1.

Both of the vendor packages are required for actual page content; it includes react-dom and react-router.

The main chunk is largely made up of locales (~67% of the size is from locale data for e11bc43, and ~45% of the size is from locale data for de65ce1). Another PR should ensure we only load the locale(s) we need; this would reduce the amount of data sent for locales by 95% or more.

This reduces the main bundle size from 6.07 MB to 3.65 MB but increases
overall application size from 14.04 MB to 14.18 MB.

Most of the remainder of the main bundle size is from polyfills
(2.35 MB) and libraries supporting localization (i18n).

Signed-off-by: Taylor Smock <tsmock@meta.com>
@sonarcloud
Copy link

sonarcloud bot commented Jun 13, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.1% 0.1% Duplication

@tsmock tsmock marked this pull request as ready for review June 13, 2023 14:35
Copy link
Contributor

@HelNershingThapa HelNershingThapa left a comment

Choose a reason for hiding this comment

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

This is great! I'm impressed with the reduction in the initial page load size you achieved with your changes. I have a couple of curiosities regarding your approach:

I'm curious about your thought process behind merging similar chunks into one (like "CreateLicense," "ListLicenses," and "EditLicense" into a single "license" chunk). I understand that we could have generated separate chunks for each route, but I'm interested in understanding the rationale behind your choice.

Additionally, in the PR description, the build time for e11bc43 is 237.6 seconds, which seems unusually high compared to the 61.5 seconds I observed when generating the build. Could you provide insights on why there might be such a significant difference in build times?

@tsmock
Copy link
Contributor Author

tsmock commented Jun 15, 2023

I'm curious about your thought process behind merging similar chunks into one

I just split the routes into file-based chunks. Since (in your example) CreateLicense, ListLicense, and EditLicense were in the same file (views/licenses.js), I just kept them in the same chunk. Realistically, we could probably split ListLicense into its own, since it probably doesn't need the components that CreateLicense and EditLicense need.

I think a bit more work needs to be done for that (specifically, it looks like I need to add some magic comments for webpackExports EDIT: not needed, webpack 5.80+ will do it automatically). I'll fiddle with that for the the views that won't necessarily need the editing components (I don't think it would be worthwhile to split out CreateLicense and EditLicense into their own chunks, but it might be worthwhile to split ListLicense out).

Could you provide insights on why there might be such a significant difference in build times?

Best guess: I'm not just working on the TM, so it is possible that another project I'm working on takes enough resources that a run might be significantly longer than another. I did note that another run took ~77s for e11bc43. But it usually takes ~3 minutes on my system.

Other possible reasons why there may have been a significant difference:

  • The IDEs might have been doing something in the background that affected build time.
  • Microsoft Defender was scanning the build files as it was building.
  • Other random mandatory processes on the system interfering with the build.

@tsmock
Copy link
Contributor Author

tsmock commented Jun 15, 2023

I think a bit more work needs to be done for that (specifically, it looks like I need to add some magic comments for webpackExports)

From the docs

Available since webpack 5.0.0-beta.18

We need webpack 5, which comes as part of #5908. I think I can add the magic comments before we update react-scripts, it just won't have any effect.

EDIT: It looks like the magic comments aren't necessary. See webpack/webpack#16995. It just isn't documented yet.

@tsmock
Copy link
Contributor Author

tsmock commented Jun 15, 2023

It doesn't look like splitting out additional chunks is currently possible; it might be possible with "type": "module" in package.json, but that doesn't work for us right now.

@HelNershingThapa
Copy link
Contributor

Thank you for the explanation. I appreciate your approach to merging similar chunks based on file structure. It makes sense to keep related routes together in the same chunk, and splitting out ListLicense into its chunk could be beneficial if it doesn't require the components used by CreateLicense and EditLicense.

I must admit that I'm not well acquainted with these magic comments and webpack configurations myself, so your insights are valuable.

@tsmock
Copy link
Contributor Author

tsmock commented Jun 16, 2023

It makes sense to keep related routes together in the same chunk, and splitting out ListLicense into its chunk could be beneficial if it doesn't require the components used by CreateLicense and EditLicense.

Right now, we would have to move ListLicense into its own file. We might not need to in the future though. For now, I think we would only want to split up particularly large and important routes. Except the heaviest components will still be in the more important routes (for example, react-md-editor @ 1.7 MB is used by ProjectDetailPage and ProjectEdit).

I must admit that I'm not well acquainted with these magic comments and webpack configurations myself, so your insights are valuable.

Neither am I. I just did a lot of research.

@HelNershingThapa HelNershingThapa merged commit 88fbf57 into hotosm:develop Jun 22, 2023
3 checks passed
@tsmock tsmock deleted the chore/lazy-load-routes branch June 22, 2023 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants