perf(appkit-ui): enable tree-shaking with sideEffects flag and modular echarts imports#442
Open
MarioCadenas wants to merge 2 commits into
Open
perf(appkit-ui): enable tree-shaking with sideEffects flag and modular echarts imports#442MarioCadenas wants to merge 2 commits into
MarioCadenas wants to merge 2 commits into
Conversation
…r echarts imports Importing any single component from the @databricks/appkit-ui/react barrel (e.g. Button) previously forced bundlers to retain every re-exported module, including the full echarts bundle (~350KB gz), because the package declared no sideEffects field and charts/base.tsx imported the monolithic echarts entry via echarts-for-react's default export. - Declare "sideEffects": ["**/*.css"] so bundlers can drop unused modules while keeping the dist/styles.css side-effect import alive. No JS module in the package relies on import-time side effects from another module; the ECharts registration lives in the same module as BaseChart, so it is always retained when charts are used. - Switch base.tsx to echarts-for-react/lib/core + echarts/core, registering only what the option builders use: LineChart, BarChart, PieChart, ScatterChart, HeatmapChart, RadarChart; Title/Tooltip/Legend/Grid/ VisualMap components; LegacyGridContainLabel (grid.containLabel) and CanvasRenderer. Keep ECharts instance type as a type-only import. - Add mount tests for every chart family that fail on ECharts missing-registration errors (which are logged, not thrown). Co-authored-by: Isaac Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
…tions - Document on the `options` prop that only the built-in feature set is registered; extra ECharts features (dataZoom, toolbox, markLine, ...) must be registered by the consumer via `use` from "echarts/core", which requires resolving the same echarts module instance/version. - Note in the registration block that tooltip-driven axisPointer ships with TooltipComponent, and that `use()` must stay co-located with BaseChart because package.json#sideEffects declares JS modules pure. - Fix the type-only import comment: the ECharts type is used for the stored instance ref. - Add tests: custom `options` with a registered component logs no registration errors; empty data renders the "No data" fallback. Co-authored-by: Isaac Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (P-1, High)
@databricks/appkit-uihad nosideEffectsdeclaration, so webpack/esbuild/Rollup consumers had to assume every module re-exported by the/reactbarrel (src/react/index.ts) is side-effectful and keep all of them. Importing justButtonshipped the entire chart stack — including the fullechartsbundle (~350KB gz),@tanstack/react-table, andmarked+dompurify— becausecharts/base.tsximportedecharts-for-react's default entry, which imports the monolithicechartspackage.Changes
"sideEffects": ["**/*.css"]inpackages/appkit-ui/package.jsonThis tells bundlers that only CSS files have import side effects, so any JS module whose exports are unused can be dropped from the graph. Reasoning for the array (vs
false):BaseChart, so it always executes when any chart component is used.dist/styles.css(exported as./styles.css), which consumers import purely for its side effect. Declaringfalsewould let bundlers prune that import;["**/*.css"]keeps it safe.distpublish tool (tools/dist-appkit.ts) spreads the whole package.json, so the field survives into the published package.Modular ECharts imports in
charts/base.tsxSwitched from
echarts-for-react(fullechartsentry) toecharts-for-react/lib/core+echarts/core, registering exactly what the option builders inoptions.tsuse:LineChart(line + area),BarChart(vertical + horizontal),PieChart(pie + donut),ScatterChart,HeatmapChart,RadarChartTitleComponent(title),TooltipComponent,LegendComponent,GridComponent(grid/xAxis/yAxis),VisualMapComponent(heatmap color scale)LegacyGridContainLabel— the cartesian option builder setsgrid.containLabel: true, which in ECharts 6 is a separately registered feature (the full bundle auto-includes it; the mount tests caught its absence as a runtime warning)CanvasRenderer(BaseChartalways renders withrenderer: "canvas")Nothing in
options.ts/normalize.tsusesdataZoom,toolbox,markLine,dataset, or other components, so they are intentionally not registered. The ECharts instance type stays a type-onlyimport type { ECharts } from "echarts"(erased at compile time). Consumers passing customoptionsthat need extra features can register them in their own app viaecharts/core'suse()— documented in a comment at the registration site.New mount tests (
charts/__tests__/base.test.tsx)Existing chart tests only covered pure option/normalize functions and never exercised ECharts rendering. ECharts does not throw on a missing registration — it logs "Series X is used but not imported" and renders a blank chart. The new suite mounts
BaseChartfor every chart family (line, area, bar, horizontal bar, scatter, pie, donut, radar, heatmap with visualMap) under jsdom with a canvas stub and fails on any missing-registration error/warning.Verification
pnpm build— dist preserves module structure (unbundle mode); builtdist/react/charts/base.jsimports onlyecharts/coresubpaths, no bareechartsvalue import anywhere in distpnpm vitest run --project appkit-ui— 17 files, 320 tests passed (including 9 new mount tests)pnpm -r typecheck,pnpm check:fix,pnpm docs:build— cleanThis pull request and its description were written by Isaac.