CAT-1335: tree shake phosphor and chunk#16160
Conversation
|
✅ Meticulous spotted 0 visual differences across 981 screens tested: view results. Meticulous evaluated ~8 hours of user flows against your PR. Expected differences? Click here. Last updated for commit 758ce55. This comment will update as new commits are pushed. |
datahub-cyborg
Bot
added
the
needs-review
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Bundle ReportChanges will decrease total bundle size by 5.94MB (-20.1%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
AdrianMachado
left a comment
AdrianMachado
left a comment
There was a problem hiding this comment.
Take a look at my comment about the phosphor icons. Everything else seems to work based on your tests, but I think either @chriscollins3456 or @asikowitz should review it too. I'm not an expert on build tools 😅
| 'import/no-extraneous-dependencies': 'off', | ||
| 'import/no-relative-packages': 'error', | ||
| 'import/prefer-default-export': 'off', // TODO: remove this lint rule | ||
| '@typescript-eslint/no-restricted-imports': [ |
There was a problem hiding this comment.
Excellent idea, I was about to suggest this while reading the PR
| ]; | ||
|
|
||
| export const PHOSPHOR_ICONS = [ | ||
| 'Activity', |
| @@ -0,0 +1,150 @@ | |||
| /** | |||
There was a problem hiding this comment.
I don't quite understand this change.
Phosphor says it supports tree-shaking out of the box.
Claude mentions that Vite should also should support tree shaking with named imports. So I am confused why we aren't getting the benefits of that without having to create this file?
This leads me to believe that either our build tools are out of date, or are misconfigured.
If we do get that working, we can simply rewrite icon imports to the dics/csr subdirectory?
There was a problem hiding this comment.
Interesting that phosphor-icons does support tree shaking, but clearly the bundle size has reduced significantly based on the PR description. agreed that it's worth investigating
There was a problem hiding this comment.
Yeah, his approach will decrease it for sure. Like Claude basically reinvented tree shaking, but more restrictive.
There was a problem hiding this comment.
But obviously the native implementation in our build tool is nicer because it doesn't require us to alias our imports
There was a problem hiding this comment.
Yes, Phosphor does support tree-shaking but only when:
1. You import from individual CSR files:
As you said.
2. You don't use wildcard imports (import *):
We have 3 files that do import * as phosphorIcons from '@phosphor-icons/react';. Easy to fix.
3. You don't use dynamic icon resolution:
We actually have quite a lot of those.
// Icon name comes from props/API at runtime
<Icon icon="BookmarkSimple" source="phosphor" />
// Resolved dynamically from imported namespace
const IconComponent = phosphorIcons[iconName]; // Can't be tree-shakenIf we want to eliminate dynamic icon resolution, we'd need to refactor ~100 files using the Icon pattern to something like this:
// Old:
<Icon icon="MagnifyingGlass" source="phosphor" />
<Icon icon="Search" source="material" /> // hypothetical, nobody actually uses source "material"
// New:
import { MagnifyingGlass } from '@phosphor-icons/react'; // but what if the icon name comes from API?
import { Search } from '@mui/icons-material';
<Icon component={MagnifyingGlass} />
<Icon component={Search} /> Not impossible but it would be a breaking change and quite a lot of effort.
The current solution is much simpler in that it creates a curated whitelist of icons and allows us to continue the same dynamic pattern. The disadvantage is that when we add an icon, we must update the list (but the new eslint rule and potentially unit test should catch that).
BTW mui-icons are also currently not tree shaken (~5 MB minified / 1.27 MB gzipped) but that's a follow-up PR.
There was a problem hiding this comment.
It seems like what we should disallow is an import * from phosphor-icons, not all imports. Then we wouldn't have to include most of the import changes in this PR.
That being said, I'm not opposed to changing the Icon component entirely. I always thought it was weird we specified icon via string. I guess the idea was to limit what icons people could use via the component -- now you're limited to the ones provided by material + phosphor? But tbh that's an annoying restriction too -- we should be able to support custom icons as well. So I would be fine changing the 100+ files to pass the icon component in directly. AI should be able to make that change pretty easily.
There was a problem hiding this comment.
Disallowing import * from phosphor-icons is not sufficient. The core issue is dynamic icon resolution, not wildcard imports and we would still be bundling all 4k icons with named imports because:
- Dynamic resolution requires all icons in scope:
// Icon component uses dynamic lookup:
const IconComponent = phosphorIcons[iconName]; // iconName from props- Vite can't tree-shake dynamic property access - it must keep all icons since it doesn't know which
iconNamevalues will be used at runtime.
We have two options:
- Curated whitelist: Current PR. Ships faster with lower risk, no API changes needed but we would need to maintain the list (with automation).
- Native tree-shaking: Cleaner but would need to refactor the following:
- ~85 simple static patterns and ~20 conditional patterns e.g.
<Icon icon={linkIcon ?? 'ArrowRight'} .../> - 1 mapped Alchemy
Iconpattern
export const ASSERTION_TYPE_TO_ICON_MAP: Record<AssertionType, JSX.Element> = {
[AssertionType.Freshness]: <Clock size={20} />, // Direct Phosphor component
[AssertionType.Volume]: <Database size={20} />,
};- 13 maps
MODULE_TYPE_TO_ICONandDATA_TYPE_ICON_MAP
Not impossible and AI will certainly help but it does add complexity and risk to the PR.
Both approaches should work and would deliver the same savings.
What is your preference?
There was a problem hiding this comment.
I think I'm with Andrew in saying option 2 may make this PR a bit bloated, but long term its less annoying.
| const vendorSize = fs.statSync(path.join(distPath, vendorFile!)).size; | ||
|
|
||
| // Phosphor should be optimized (< 500 KB) | ||
| expect(phosphorSize).toBeLessThan(500 * 1024); |
There was a problem hiding this comment.
Will this break if we add more icons?
There was a problem hiding this comment.
yes - it's 372kb now. If we add many more, this would need to be updated.
| expect(vendorSize).toBeGreaterThan(7 * 1024 * 1024); | ||
| expect(vendorSize).toBeLessThan(10 * 1024 * 1024); |
There was a problem hiding this comment.
This is a moving target as we update these libraries, no?
datahub-cyborg
Bot
removed
the
needs-review
datahub-cyborg
Bot
added
the
pending-submitter-response
| 'framework', // React core | ||
| 'antd-vendor', // Ant Design | ||
| 'mui-vendor', // Material UI |
There was a problem hiding this comment.
these aren't the chunks based on the new chunking strategy in vite.config
| @@ -0,0 +1,147 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
where and when do we plan to run this script?
| // Step 2: Verify all imported icons are exported | ||
| console.log('=== Verifying Icon Exports ===\n'); | ||
|
|
||
| const phosphorIconsPath = path.join(__dirname, '..', 'src/alchemy-components/components/Icon/phosphor-icons.ts'); | ||
| const phosphorIconsContent = fs.readFileSync(phosphorIconsPath, 'utf8'); | ||
|
|
||
| // Extract exported icons | ||
| const exportedIcons = new Set(); |
There was a problem hiding this comment.
Step 1 and 2 are redundant imo, and should be caught by ESLint and typescript.
There was a problem hiding this comment.
also it depends on how frequently we run this script
There was a problem hiding this comment.
I think we could turn this into a unit test.
ani-malgari
left a comment
ani-malgari
left a comment
There was a problem hiding this comment.
The performance gains are already amazing. Everything looks good to me except I just left some minor comments with regards to the validation scripts.
| export { X } from '@phosphor-icons/react/dist/csr/X'; | ||
| export { XCircle } from '@phosphor-icons/react/dist/csr/XCircle'; | ||
|
|
||
| // Text formatting icons (for Editor toolbar) |
There was a problem hiding this comment.
I see no reason to distinguish between the two
| @@ -0,0 +1,150 @@ | |||
| /** | |||
There was a problem hiding this comment.
It seems like what we should disallow is an import * from phosphor-icons, not all imports. Then we wouldn't have to include most of the import changes in this PR.
That being said, I'm not opposed to changing the Icon component entirely. I always thought it was weird we specified icon via string. I guess the idea was to limit what icons people could use via the component -- now you're limited to the ones provided by material + phosphor? But tbh that's an annoying restriction too -- we should be able to support custom icons as well. So I would be fine changing the 100+ files to pass the icon component in directly. AI should be able to make that change pretty easily.
|
Closed, this one got merged instead: #16338 |
4 participants
Summary: Phase 1 of FE performance optimizations
The DataHub frontend currently produces around 20MB of minified JavaScript across 5 chunks, compressing to approximately 5 MB when gzipped. This is a substantial bundle size that impacts initial load time and performance, particularly on slower networks. Phase 1 addresses some of the issues:
vite.config.tsantdto chunksResults
Before
index-B3Suu7YI.js= 19,395 KB (~19 MB)After
vendor-yJ7kOkOV.js: 8.6 MB (8,999.72 KB) [KEY CHUNK]
source-CbhsHxku.js: 3.9 MB (4,042.65 KB)
phosphor-vendor-D8fS90Jx.js: 372 KB (408.51 KB) [OPTIMIZED]
index-CxYYPpoy.js: 4 KB (0.74 KB)
Total Size:
Bundle Analyzer
rollup-plugin-visualizeinstalled as a dev dependencyyarn build:analyzeandopen dist/stats.htmlRisks
→ Mitigation:
npm run validate-bundle→ Mitigation: added
datahub-web-react/src/__tests__/bundle-optimization.test.tsReference
https://www.notion.so/acryldata/DataHub-Frontend-Performance-Tech-Plan-246fc6a64277803e9976fdde565602d8