-
Notifications
You must be signed in to change notification settings - Fork 4k
Reduce entry bundle size - Part 4 #13115
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
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
✅ PR preview is ready!
|
📉 Significant wheel size change detectedThe wheel file size has decreased by 0.26% (threshold: 0.25%)
Please verify that this change is expected. |
📉 Significant bundle size change detected
Please verify that this change is expected. |
0457491 to
5183381
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR successfully reduces the entry bundle size from 2.38 MB to 2.15 MB by removing apache-arrow from the entry bundle. It achieves this by refactoring ElementNode to defer Quiver instantiation to component-level, storing raw Arrow proto data instead.
Key Changes
- Refactored
ElementNodeto store raw Arrow proto data and accumulateaddedRowsListinstead of instantiating Quiver eagerly - Moved Quiver instantiation logic from
ElementNodeto components (DataFrame,ArrowTable,ArrowVegaLiteChart) - Added new utility functions in
dataframeUtils.tsfor consistent Quiver creation and merging - Lazy-loaded
ComponentInstanceandFormSubmitContentfor additional bundle size reduction - Updated error messages to remove stack traces for cleaner UI display
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/lib/src/render-tree/ElementNode.ts |
Refactored to store raw Arrow proto data and addedRowsList instead of instantiating Quiver; removed merge helper functions |
frontend/lib/src/render-tree/ElementNode.test.ts |
Updated tests to verify raw data storage instead of Quiver instantiation |
frontend/lib/src/dataframes/dataframeUtils.ts |
New utility file with functions for Quiver creation and merging |
frontend/lib/src/dataframes/dataframeUtils.test.ts |
Comprehensive tests for new utility functions |
frontend/lib/src/dataframes/arrowConcatUtils.ts |
Refactored error messages to single-line format without stack traces |
frontend/lib/src/dataframes/__snapshots__/Quiver.test.ts.snap |
Updated snapshots reflecting new error message format |
frontend/lib/src/dataframes/Quiver.ts |
Added createError method for user-facing errors without stack traces |
frontend/lib/src/components/widgets/DataFrame/ReadOnlyGrid.tsx |
Updated to accept IArrow | Quiver and extract data bytes for DataFrame |
frontend/lib/src/components/widgets/DataFrame/ReadOnlyGrid.test.tsx |
Updated tests to pass IArrow instead of Quiver instances |
frontend/lib/src/components/widgets/DataFrame/DataFrame.tsx |
Added useMemo to instantiate Quiver from proto and merge addedRowsList |
frontend/lib/src/components/widgets/DataFrame/DataFrame.test.tsx |
Updated to pass raw data instead of Quiver instances in props |
frontend/lib/src/components/elements/ArrowVegaLiteChart/arrowUtils.ts |
Added buildVegaLiteChartElement function to construct VegaLiteChartElement from proto and added rows |
frontend/lib/src/components/elements/ArrowVegaLiteChart/arrowUtils.test.ts |
Added comprehensive tests for buildVegaLiteChartElement |
frontend/lib/src/components/elements/ArrowVegaLiteChart/ArrowVegaLiteChart.tsx |
Updated to use buildVegaLiteChartElement and pass raw data to ReadOnlyGrid |
frontend/lib/src/components/elements/ArrowVegaLiteChart/ArrowVegaLiteChart.test.tsx |
Updated to pass proto instead of VegaLiteChartElement |
frontend/lib/src/components/elements/ArrowTable/ArrowTable.tsx |
Added useMemo to instantiate Quiver from proto and merge addedRowsList |
frontend/lib/src/components/elements/ArrowTable/ArrowTable.test.tsx |
Updated to pass raw data in proto instead of separate Quiver instances |
frontend/lib/src/components/core/Block/ElementNodeRenderer.tsx |
Updated to extract raw data from ElementNode and lazy-load ComponentInstance and FormSubmitContent |
e2e_playwright/st_add_rows_test.py |
Updated test selector from stAlert to more specific stAlertContentError |
Comments suppressed due to low confidence (1)
frontend/lib/src/dataframes/Quiver.ts:320
- This string appears to be missing a space after 'of'.
" If you do not need the Styler's styles, try passing the .data attribute of" +
frontend/lib/src/components/elements/ArrowVegaLiteChart/arrowUtils.ts
Outdated
Show resolved
Hide resolved
5183381 to
7fc4df5
Compare
a6e325e to
9cda13b
Compare
📉 Frontend coverage change detectedThe frontend unit test (vitest) coverage has decreased by 0.0100%
✅ Coverage change is within normal range. |
7fc4df5 to
6b561b7
Compare
6b561b7 to
a7ec894
Compare
| @@ -81,8 +80,6 @@ import { Skeleton } from "~lib/components/elements/Skeleton" | |||
| import TextElement from "~lib/components/elements/TextElement" | |||
| import ErrorBoundary from "~lib/components/shared/ErrorBoundary" | |||
| import Heading from "~lib/components/shared/StreamlitMarkdown/Heading" | |||
| import { ComponentInstance } from "~lib/components/widgets/CustomComponent" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the couple added lazy loads, this was just reorganizing components back into alphabetical order under each elements/widgets comment
| @@ -299,23 +299,28 @@ export class Quiver { | |||
| return this._data.getChildAt(columnIndex)?.get(rowIndex) | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These errors are now rendered through the ErrorBoundary in ElementNodeRenderer which does not support markdown in the message.
|
We just deprecated the Maybe if possible we can prioritize the #13128 and |
a7ec894 to
930a516
Compare
lukasmasuch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍

Describe your changes
Attempt to improve initial load time by reducing dependencies included in entry bundle.
Changes made:
Removeapache-arrowfrom entry bundleadd_rowsdeprecationRefactorElementNodeto moveQuiverinstantiation logic into componentsLazy load
FormSubmitContent&ComponentInstanceBundle Analysis:
Before: 2.38 MB & After: 2.36 MB