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

Lazy load components #3879

Merged
merged 21 commits into from Jul 7, 2017

Conversation

@sorin-davidoi
Copy link
Collaborator

commented Jun 20, 2017

Roadmap:

  • Display progress bar while fetching bundles
  • Routes
    • Lazy load (react-router guide)
    • Loading state
      • Empty column
      • Move styles to SCSS
    • Replay request (if bundle could not be fetched)
  • Columns:
    • Compose (+ reducer)
    • Home
    • Notifications (+ reducer)
    • Public timeline
    • Community timeline
    • Hashtag timeline
    • Loading state - empty column
  • Modals
    • Components:
      • Media
      • Onboarding
      • Video
      • Boost
      • Confirm
      • Report
    • Replay request (if bundle could not be fetched)
    • Move styles to SCSS
  • Do not show the waiting state if the bundle is fetched fast enough (~200ms)
  • Optimize bundles - perhaps tweak minChunks?
  • Measure size improvement

Bundles: https://gist.github.com/sorin-davidoi/07def6eb30df015fbfc8ced9fa36babd

@sorin-davidoi sorin-davidoi changed the title Lazy load components [WIP] Lazy load components Jun 20, 2017
@sorin-davidoi sorin-davidoi changed the title [WIP] Lazy load components Lazy load components Jun 21, 2017
@sorin-davidoi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 21, 2017

This reduces the size of application.js by 27% (673kB -> 485kb), while creating ~25 extra bundles. The progress bar is being used to show the download progress of each bundle. While the bundle is fetched, it displays a loading state in accordance with the content. If the fetch fails, an error component is shown, which provides a "refresh" button, allowing the user to replay the download.

@nolanlawson

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2017

Nice work! Based on the Webpack Bundle Analyzer output, though, it does appear that there are lots of common modules that are repeated throughout the async chunks – status.js, video_player.js, react-simple-dropdown, react-router-scroll, etc. I suppose WebpackCommonsChunkPlugin doesn't include those if they're included in async chunks? It seems useful to put those in common.js, given that there will probably be lots of users who load at least two of home/localTL/federatedTL/accountTL/hashtagTL/etc.

@nightpool

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

I'm not sure I see the practical benefit of this—won't it just increase latency on poor connections due to the extra round trip requests?

Is there a way to measure whether the decreased bundle size will actually reflect real world usage?

@sorin-davidoi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

It should decrease time to first paint (less JavaScript to parse, which is the main bottleneck on mobile), as well as memory usage. Don't think we are currently measuring those?

@sorin-davidoi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

As a side note, it would be nice to set up Lighthouse integration, to measure these metrics for each PR.

@sorin-davidoi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

Unscientific findings (running Lighthouse against current master and this branch):

  • Performance score: +3 (51 -> 54)
  • Render blocking scripts: -200ms (2070ms -> 1820ms)
  • First paint: -140ms (7060ms -> 6950ms)
  • First interactive: -600ms (7570ms -> 6950ms)

I would prefer to take the average of a few tests but I don't have the time to do it (is there a tool for it?).

@nolanlawson

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

won't it just increase latency on poor connections due to the extra round trip requests?

That can be fixed by adding <link rel=preload>s for each of the chunks that we expect are very likely to get loaded by the user. I think this would be a smart addition; @sorin-davidoi you can add it to application.html.haml like so:

%link{:rel => "preload", :as => "script", :src => (asset_pack_path 'application.js')}/

Also for your Lighthouse score, what page did you test against? There are a few I think we want to prioritize:

  • /web/getting-started – main home
  • /web/timelines/home – mobile PWA home (defined in manifest.json)

I also imagine there are people who bookmark local/federated/home timelines, so those would be worthwhile to preload as well.

Overall, I believe it is worthwhile to only load those routes that the user explicitly requested, since it limits overhead of JS parse/exec (and also memory). But avoiding extra network roundtrips is important, and also I'd like to see some deduplication of the shared modules across async chunks.

@sorin-davidoi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

I tested the local timeline (mobile UI). <link rel=preload> sounds nice, will try to add them! 👍

@sorin-davidoi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

I think we could also lazy-load some reducers (see https://stackoverflow.com/questions/32968016/how-to-dynamically-load-reducers-for-code-splitting-in-a-redux-application), but that is a can of worms I don't want to open for now. Proceeds to open can of worms.

@beatrix-bitrot

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2017

Is this ready for review?

@sorin-davidoi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

Just a few final touches to do. Found a crash as well.

@sorin-davidoi

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

I'd say it is now ready for review. Could lazy-load more reducers I think but I would rather do it in another PR. Would also be nice to lazy-load the Emoji picker using the same idioms used here, but that can wait as well.

@beatrix-bitrot

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2017

nice, because it's a big PR and not my area of expertise I will test this branch on glitch.social to make my review

Copy link
Collaborator

left a comment

Codewise this looks great to me; I just have a few small suggestions. This is also deployed at malfunctioniong.technology (with service worker as well) for those who want to take a look.


// Dummy import, to make sure that <Status /> ends up in the application bundle.
// Without this it ends up in ~8 very commonly used bundles.
import '../../components/status';

This comment has been minimized.

Copy link
@nolanlawson

nolanlawson Jun 23, 2017

Collaborator

nice fix


= javascript_pack_tag 'features/community_timeline', integrity: true, crossorigin: 'anonymous', rel: 'preload'

= javascript_pack_tag 'features/public_timeline', integrity: true, crossorigin: 'anonymous', rel: 'preload'

This comment has been minimized.

Copy link
@nolanlawson

nolanlawson Jun 23, 2017

Collaborator

These should have as="script" as well; it's explained what the browser does with this extra information here.

@nolanlawson

This comment has been minimized.

Copy link
Collaborator

commented Jun 23, 2017

In terms of bundle sizes this also looks great. I rebuilt and put a Webpack Bundle Analyzer view here for those who want to see. (Protip: make a Gist with an index.html and bl.ocks.org can host it for you. 😃)

@Gargron

This comment has been minimized.

Copy link
Member

commented Jun 25, 2017

@nolanlawson Waiting on you to check the changes.

Copy link
Collaborator

left a comment

This looks great to me, and is live on malfunctioning.technology to test. (You may need to hard-refresh due to ServiceWorker.)

We may want to warn admins when we ship this that they will probably want to enable HTTP/2 since we are now preloading 13 JS files, which will be slow on HTTP/1 due to the 6-connections-per-origin limit. We may also want to tweak the preloading to limit it to only those components that are guaranteed to be loaded (e.g. "getting started" will only be loaded for first-time users; maybe we can detect that somehow with cookies?).

But those are minor concerns; overall this looks awesome to me and I'm 👍 to merge. I especially love the new async-components.js so we can keep all of those components in one place. Great work!

@nolanlawson

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2017

BTW http2 is recommended in the tootsuite/documentation nginx config so hopefully most admins already have it.

@unarist

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2017

Component which is loaded before also seem to be lazy loaded.

image

I think it can be avoided by caching result of import(). (sample: unarist@4cf1fde)

image

Pros: avoid flashing of loading view, slightly reduces total time
Cons: single script execution will be longer, i.e. blocks browser longer

How about this?

@sorin-davidoi sorin-davidoi referenced this pull request Jul 7, 2017
1 of 2 tasks complete
@Gargron Gargron merged commit 348d6f5 into tootsuite:master Jul 7, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Gargron

This comment has been minimized.

Copy link
Member

commented Jul 8, 2017

@sorin-davidoi Please guide me through where the "loading" and "error" views are defined and how to get to see them without a slow connection so I can make sure I like how they look visually.

@sorin-davidoi

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2017

@Gargron Open the Network tab in the Chrome development tools and tick "Disable cache".

For columns:

  • Loading state - Load a timeline, throttle the connection to GPRS, click on a toot.
  • Error state - Load a timeline, tick the "Offline" checkbox, click on a toot. After the error state is shown you can uncheck "Offline" and click retry, the normal page should appear.

For modals:

  • Loading state - throttle the connection to GPRS and open a media modal
  • Error state - tick "Offline" and open a media modal - same behavior with retry
@sorin-davidoi sorin-davidoi deleted the sorin-davidoi:lazy-loading branch Jul 8, 2017
rtucker added a commit to vulpineclub/mastodon that referenced this pull request Jul 8, 2017
* feat: Lazy-load routes

* feat: Lazy-load modals

* feat: Lazy-load columns

* refactor: Simplify Bundle API

* feat: Optimize bundles

* feat: Prevent flashing the waiting state

* feat: Preload commonly used bundles

* feat: Lazy load Compose reducers

* feat: Lazy load Notifications reducer

* refactor: Move all dynamic imports into one file

* fix: Minor bugs

* fix: Manually hydrate the lazy-loaded reducers

* refactor: Move all dynamic imports to async-components

* fix: Loading modal style

* refactor: Avoid converting the raw state for each lazy hydration

* refactor: Remove unused component

* refactor: Maintain modal name

* fix: Add as=script to preload link

* chore: Fix lint error

* fix(components/bundle): Check if timestamp is set when computing elapsed

* fix: Load compose reducers for the onboarding modal
Gargron added a commit that referenced this pull request Jul 10, 2017
* Add Japanese translations for #3879

* Add Japanese translations for #4033

* Add Japanese translations for #4136
rtucker added a commit to vulpineclub/mastodon that referenced this pull request Jul 10, 2017
* feat: Lazy-load routes

* feat: Lazy-load modals

* feat: Lazy-load columns

* refactor: Simplify Bundle API

* feat: Optimize bundles

* feat: Prevent flashing the waiting state

* feat: Preload commonly used bundles

* feat: Lazy load Compose reducers

* feat: Lazy load Notifications reducer

* refactor: Move all dynamic imports into one file

* fix: Minor bugs

* fix: Manually hydrate the lazy-loaded reducers

* refactor: Move all dynamic imports to async-components

* fix: Loading modal style

* refactor: Avoid converting the raw state for each lazy hydration

* refactor: Remove unused component

* refactor: Maintain modal name

* fix: Add as=script to preload link

* chore: Fix lint error

* fix(components/bundle): Check if timestamp is set when computing elapsed

* fix: Load compose reducers for the onboarding modal
YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017
* feat: Lazy-load routes

* feat: Lazy-load modals

* feat: Lazy-load columns

* refactor: Simplify Bundle API

* feat: Optimize bundles

* feat: Prevent flashing the waiting state

* feat: Preload commonly used bundles

* feat: Lazy load Compose reducers

* feat: Lazy load Notifications reducer

* refactor: Move all dynamic imports into one file

* fix: Minor bugs

* fix: Manually hydrate the lazy-loaded reducers

* refactor: Move all dynamic imports to async-components

* fix: Loading modal style

* refactor: Avoid converting the raw state for each lazy hydration

* refactor: Remove unused component

* refactor: Maintain modal name

* fix: Add as=script to preload link

* chore: Fix lint error

* fix(components/bundle): Check if timestamp is set when computing elapsed

* fix: Load compose reducers for the onboarding modal
YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017
* Add Japanese translations for tootsuite#3879

* Add Japanese translations for tootsuite#4033

* Add Japanese translations for tootsuite#4136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.