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

More lazy loading of react components to reduce bundle size #1888

Merged
merged 47 commits into from
Apr 21, 2021
Merged

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Apr 8, 2021

Currently loading the page load

  • For start screen incurs 3.8MB js to be downloaded
  • For a page direct to an LGV with a track incurs about 7.4MB js

This PR does not help too much yet but I thought some more work to reduce JS size could be beneficial

  • Making some of the NewSessionCard images smaller and runs pngquant on them (low hanging fruit, can be made a separate PR probably)
  • (Re-) introducing the LazyReactComponent option for ViewType, which could be expanded upon. I added LazyReactComponent for circular, dotplot, and spreadsheet
  • For spreadsheet in particular, tries to lazy load the parsers since csvtojson dependency is ~160kb
  • Makes dotplot/circular/spreadsheet not use jbrequire just for simplicity and since it doesn't appear needed

Note that the lazy loading isn't paying off big yet but I think it could help in the long run cut our load times

Current payloads with this PR

  • For a page direct to an LGV with a track 6.4MB (delta -1MB)
  • For a start screen 3.3MB (delta -0.5MB)

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Apr 8, 2021
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 8, 2021

sv inspector needs a fix to use LazyReactComponent so that's on the todo, note still draft PR

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 8, 2021

MB size calculated by filtering on /.js$/ in the network tab in chrome

@cmdcolin cmdcolin added performance and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Apr 8, 2021
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Apr 9, 2021
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 9, 2021

Some new figures made by kicking some of the widget code into lazy loading

  • LGV with track loaded 5.4MB (delta -2MB)
  • Empty session 2.8MB (delta -1MB)

@codecov
Copy link

codecov bot commented Apr 9, 2021

Codecov Report

Merging #1888 (2a985e0) into main (c531991) will decrease coverage by 0.53%.
The diff coverage is 36.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1888      +/-   ##
==========================================
- Coverage   58.13%   57.59%   -0.54%     
==========================================
  Files         461      458       -3     
  Lines       21488    21342     -146     
  Branches     5102     5094       -8     
==========================================
- Hits        12491    12291     -200     
- Misses       8686     8741      +55     
+ Partials      311      310       -1     
Impacted Files Coverage Δ
packages/core/BaseFeatureWidget/index.ts 75.00% <ø> (ø)
packages/core/PluginManager.ts 94.23% <ø> (ø)
packages/core/pluggableElementTypes/ViewType.ts 81.81% <ø> (ø)
packages/core/pluggableElementTypes/WidgetType.ts 81.81% <ø> (ø)
...ore/pluggableElementTypes/models/BaseTrackModel.ts 33.72% <ø> (-3.65%) ⬇️
packages/core/ui/Drawer.js 100.00% <ø> (ø)
packages/core/ui/RecentSessionCard.js 14.28% <ø> (ø)
packages/core/ui/Tooltip.tsx 15.38% <ø> (-11.29%) ⬇️
packages/core/util/types/index.ts 61.53% <ø> (ø)
...ns/alignments/src/AlignmentsFeatureDetail/index.js 75.00% <ø> (ø)
... and 105 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c531991...2a985e0. Read the comment docs.

@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Apr 9, 2021
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 9, 2021

Appears to need fixing for plugin builds, tried adding inlineDynamicImports to the packages/development-tools rollup config but not there yet. That does imply plugin builds wouldn't use code splitting

Also there is an alt branch that extends this PR by making data adapters import their parsers on the fly too
lazyreact...lazyreact_dataadapters

That makes it so that for example you don't import the @gmod/bam until you actually load a BAM track (The BamAdapter is actually still loaded, but the @gmod/bam is delayed...there may be another way to approach that too that doesn't require so much digging into the data adapters where the adapter itself is lazily loaded)

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Apr 15, 2021
@cmdcolin cmdcolin changed the title More lazy loading of react components, aim to reduce bundle size More lazy loading of react components to reduce bundle size Apr 15, 2021
@garrettjstevens garrettjstevens self-requested a review April 15, 2021 23:07
Copy link
Collaborator

@garrettjstevens garrettjstevens left a comment

Choose a reason for hiding this comment

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

I haven't had a chance to go through this thoroughly yet, but see a couple initial comments. Also a couple questions:

  • If e.g. a widget's ReactComponent is not lazy, will the Suspense handle that ok?
  • Does this mean that any product that uses e.g. a plugin with a ViewType will need to wrap the ReactComponent in a Suspense? Can we make TypeScript aware of that by e.g. using LazyExoticComponent in the ViewType.ts base class?

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 16, 2021

If e.g. a widget's ReactComponent is not lazy, will the Suspense handle that ok?

yes, the Suspense is pretty flexible, see here https://reactjs.org/docs/code-splitting.html (specifically how normal jsx, and several lazy components are in that one example)

Does this mean that any product that uses e.g. a plugin with a ViewType will need to wrap the ReactComponent in a Suspense? Can we make TypeScript aware of that by e.g. using LazyExoticComponent in the ViewType.ts base class?

I think that is a good idea, testing it out now...

@cmdcolin
Copy link
Collaborator Author

Ended up needing, at least from cursory look, LazyExoticComponent<React.FC<...>>|React.FC<...> to properly type it being lazy or not

@rbuels rbuels added internal and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Apr 19, 2021
Copy link
Contributor

@rbuels rbuels left a comment

Choose a reason for hiding this comment

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

A few little nits to pick, comments and questions, but ok to merge as is.

packages/core/BaseFeatureWidget/index.test.js Outdated Show resolved Hide resolved
packages/core/ui/NewSessionCards.tsx Outdated Show resolved Hide resolved
packages/core/ui/NewSessionCards.tsx Outdated Show resolved Hide resolved
packages/core/ui/NewSessionCards.tsx Outdated Show resolved Hide resolved
packages/core/ui/NewSessionCards.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
declare module '@jbrowse/core/ui/RecentSessionCard'
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved packages/core/ui/StartScreen.tsx to products/jbrowse-desktop/src/StartScreen.tsx and correspondingly needed to move a declare.d.ts for this item there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did this because jbrowse-web already had it's own specialized StartScreen so I figured there was no need to have one in core

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.

3 participants