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

feat: Lazy loading and code splitting #1802

Merged
merged 16 commits into from
Feb 26, 2024

Conversation

mattrunyon
Copy link
Collaborator

@mattrunyon mattrunyon commented Feb 14, 2024

This adds lazy loading for chart and iris-grid and enables it for MarkdownComponent as well.

Replaced the exports for Chart and IrisGrid with lazy loading wrappers. This should mean anywhere else we use the components in the app should be automatically lazy loaded.

iris-grid doesn't seem to really lazy load anything. Might be that most/all of its dependencies are already used in other spots in the app, so they all load anyway.

chart I confirmed lazy loads by watching the network tab on page load with no charts open. When I open a chart or view the memory usage graph, the network tab shows 2 chunks loading in including plotly.

MarkdownComponent I confirmed the same was as chart but with opening a markdown panel. There is a big chunk lazy loaded which includes mathjax.

We could remove the iris-grid stuff for now if we want, or keep it and maybe future changes will enable splitting. Outside of monaco I don't think it's a very heavy import

@mattrunyon mattrunyon self-assigned this Feb 14, 2024
Copy link

codecov bot commented Feb 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 45.98%. Comparing base (6b4712b) to head (a506aec).
Report is 10 commits behind head on main.

Files Patch % Lines
packages/iris-grid/src/LazyIrisGrid.tsx 0.00% 4 Missing ⚠️
packages/chart/src/LazyChart.tsx 0.00% 3 Missing ⚠️
packages/chart/src/plotly/LazyPlot.tsx 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1802      +/-   ##
==========================================
- Coverage   46.06%   45.98%   -0.09%     
==========================================
  Files         627      631       +4     
  Lines       37792    37832      +40     
  Branches     9522     9529       +7     
==========================================
- Hits        17410    17397      -13     
- Misses      20328    20381      +53     
  Partials       54       54              
Flag Coverage Δ
unit 45.98% <0.00%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dsmmcken
Copy link
Contributor

Good work.

monaco will basically always be required because of the console

An app mode dashboard can disable the console, and is actually probably the expected pattern. Iris grid sidebar does use Monaco for custom columns, and that would benefit from lazy loading in monaco.

@mofojed
Copy link
Member

mofojed commented Feb 14, 2024

monaco will basically always be required because of the console.

That's not true - in App mode, you can specify a default layout without a console at all. We would need console to lazy load it as well too though, and would need to initialize Monaco correctly still (AppRoot calls MonacoUtils.init right now)

packages/chart/src/plotly/Plot.tsx Outdated Show resolved Hide resolved
packages/chart/src/LazyChart.tsx Show resolved Hide resolved
@mofojed
Copy link
Member

mofojed commented Feb 14, 2024

Seems to have broken the Style guide?

@mattrunyon
Copy link
Collaborator Author

Added @bmingles to confirm he's ok with the CSS fix for the chunks loading out of order in production

@mattrunyon
Copy link
Collaborator Author

Also, I didn't think about app mode not necessarily requiring a monaco load. That said, I won't worry about splitting out monaco in this PR as I mostly wanted mathjax and plotly split

@mattrunyon
Copy link
Collaborator Author

Added an e2e test which works because of the manual chunks for the large packages. Tested the e2e test locally by changing the LazyPlot export in the chart index to just export from Plot which causes plotly to not lazy load and the test to fail

Comment on lines 4 to 6
/**
* Intentionally using the classname twice so we have higher specificity than spectrum's definitions
*/
Copy link
Member

Choose a reason for hiding this comment

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

Why are we doing this and why is it a part of this change? Should add to the comment why we need to do this (chunks loading out of order).

Though that being said - chunks loading out of order implies there's something off with the packaging. Why is this happening now?

Copy link
Collaborator Author

@mattrunyon mattrunyon Feb 20, 2024

Choose a reason for hiding this comment

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

I'll update the comment.

I didn't dig too far into why the chunks were out of order (we don't directly import spectrum's css nor can we because it's spread across every component). We could also have an import order wrong somewhere in our code. Or because of lazy loading the chunks are getting split a little differently. We still put node_modules into 1 of 4 chunks (monaco, plotly, mathjax, vendor). Everything else seems to chunk based on the dynamic imports.

Either way, just increasing the specificity is safer in the long run I think (although it did fail e2e tests because of the import order)

Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

LGTM

@mattrunyon mattrunyon merged commit 25d1c09 into deephaven:main Feb 26, 2024
4 of 5 checks passed
@mattrunyon mattrunyon deleted the code-splitting branch February 26, 2024 18:33
@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants