Reduce JS production build size by ~25% #13912
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughChanges include webpack and babel configuration updates for build optimization, refactoring Vue Intl locale data loading to be dynamic and lazy-loaded based on locale parameter, removing explicit core-js polyfill imports in favor of configuration-driven injection, and updating i18n setup to load locale data asynchronously. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/kolibri-build/src/webpack.config.base.js (1)
110-117: Enabling Terser mangling: confirm no code relies on function/class namesTurning on
mangle(with Safari 10 safeguards) is a solid win for bundle size, but it can break any code that inspectsFunction.name/constructor.nameor otherwise depends on stable symbol names at runtime. Please verify there are no such dependencies (including in third‑party libs you bundle) and that the production build still works across your supported browsers. The duplicatedsafari10setting (insidemangleand at the top level) is redundant but harmless if you prefer to keep it.packages/kolibri-i18n/src/intl_code_gen.js (1)
5-6: Deduping vue‑intl locales by base code withuniqByis appropriateUsing
uniqBy(languageInfo, l => l.intl_code.split('-')[0])keeps only one entry per base locale (e.g.en,es), which matches vue‑intl’s use of base codes and avoids generating duplicatecaseblocks. Note thatgenerateVueIntlItemsreturnsundefinedforachand any intentionally skipped locales;Array.prototype.join('')will effectively drop those entries, so behavior is correct. If you want to make this more explicit, you could optionally filter truthy values beforejoin:uniqBy(languageInfo, l => l.intl_code.split('-')[0]) .map(generateVueIntlItems) .filter(Boolean) .join('')Also applies to: 128-134
packages/kolibri/utils/i18n.js (2)
234-234: Consider using the existinglanguageIdToCodehelper.The inline
split('-')[0]duplicates the logic from thelanguageIdToCodefunction (lines 133-135). Using the existing helper would follow DRY principles.Apply this diff:
- const vueIntlLanguageCode = currentLanguage.split('-')[0]; + const vueIntlLanguageCode = languageIdToCode(currentLanguage);
235-236: Consider renamingrequireLocaleDatafor clarity.The variable
requireLocaleDatais the resolved function from the Promise, but the name reads like an action. Consider a name that better conveys it's a loader function, such asgetLocaleDataorlocaleDataLoader.Example:
- return importVueIntlLocaleData(vueIntlLanguageCode).then(requireLocaleData => { - const localeData = requireLocaleData(); + return importVueIntlLocaleData(vueIntlLanguageCode).then(getLocaleData => { + const localeData = getLocaleData();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/kolibri-build/src/webpack.config.base.js(1 hunks)packages/kolibri-i18n/src/intl_code_gen.js(3 hunks)packages/kolibri-sandbox/babel.config.js(1 hunks)packages/kolibri-sandbox/src/iframe.js(0 hunks)packages/kolibri-sandbox/src/xAPI/xAPIConstants.js(0 hunks)packages/kolibri-sandbox/src/xAPI/xAPISchema.js(0 hunks)packages/kolibri-sandbox/webpack.config.js(1 hunks)packages/kolibri/composables/useUser.js(1 hunks)packages/kolibri/utils/i18n.js(3 hunks)packages/kolibri/utils/internal/vue-intl-locale-data.js(1 hunks)
💤 Files with no reviewable changes (3)
- packages/kolibri-sandbox/src/xAPI/xAPISchema.js
- packages/kolibri-sandbox/src/xAPI/xAPIConstants.js
- packages/kolibri-sandbox/src/iframe.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: All file linting
- GitHub Check: Frontend tests
- GitHub Check: Build WHL file / Build WHL
🔇 Additional comments (7)
packages/kolibri/composables/useUser.js (1)
11-11: Excellent fix for bundle size reduction!This change from
'lodash'to'lodash/pick'is the critical fix mentioned in the PR description. Importing from the main lodash entry point bundles the entire library (~70KB+ minified), while the subpath import only includes thepickfunction (~1-2KB), enabling proper tree-shaking.packages/kolibri-sandbox/babel.config.js (1)
6-7: Polyfill strategy switch touseBuiltIns: 'usage'with core‑js 3.46This is a sensible way to keep sandbox polyfills lean while relying on Babel to inject only what’s used. Please double‑check that
core-js3.46 is installed in this package’s dependency graph and that the sandbox still behaves correctly on older/Safari browsers now that you’re depending on preset‑env’s injected imports instead of explicit core‑js imports.packages/kolibri-sandbox/webpack.config.js (1)
40-43: Simplified JS loaderexcludeis conventional; verify node_modules don’t need transpilationUsing
exclude: /node_modules/here is a straightforward way to ensure only sandbox sources go through Babel, which should help keep the config simple and the build lean. Just confirm that none of the JS you pull fromnode_modulesships untranspiled syntax that your target browsers (especially Safari) can’t handle, since those files will now bypass Babel.packages/kolibri-i18n/src/intl_code_gen.js (1)
53-55: New async vue‑intl loader generation: ensure callers pass base locale and handle PromisesThe generated module now exports
function (locale) { switch (locale) { … } }where each case returns aPromisethat resolves to a loader for the correct vue‑intl locale data (with polyfill fallback via./polyfills/vue-intl/*.jswhen necessary). This looks consistent with the goal of per‑locale code‑splitting. Please verify that:
- Callers now treat
vue-intl-locale-data.jsas an async API (await/then) rather than expecting a synchronously populated array, and- The
localepassed at runtime is the base code (intl_code.split('-')[0], e.g.'en','es'), matching thecase '${vue_intl_code}'generated here; otherwise they’ll fall through to the default'en'case.Also applies to: 58-69, 101-110, 114-124
packages/kolibri/utils/internal/vue-intl-locale-data.js (1)
11-205: Async, per‑locale vue‑intl loader: confirm all call sites are updatedThis generated module now exposes a Promise‑based, per‑locale loader using
require.ensurefor each vue‑intl locale (with a polyfill forhtand a default fallback to English). The structure matches the updated code generator and is well‑suited for code‑splitting. Please confirm that:
- All consumers of
packages/kolibri/utils/internal/vue-intl-locale-data.jsnow call it with a locale argument and handle the returned Promise, and- The locale value is normalized to the same base codes used here (
'ar','bn','pt', etc.), so you consistently hit the intendedcaseinstead of the default.Given the PR goal, it would also be good to explicitly exercise a few locales (including ones using the polyfill path like
ht) in Safari to ensure that async loading and polyfilling behave as expected.packages/kolibri/utils/i18n.js (2)
232-239: LGTM! Async locale data loading implemented correctly.The asynchronous loading of vue-intl locale data successfully implements code-splitting as intended in the PR objectives. Key aspects:
- The
_i18nReadyflag is correctly set only after locale data loads, preventing premature use of translation functions- Returning the Promise enables proper chaining in
i18nSetup- Error handling will propagate correctly through the Promise chain
255-255: LGTM! Promise chaining ensures i18n is fully ready.The updated calls to
_setUpVueIntl().then(resolve, reject)correctly ensure that thei18nSetupPromise doesn't resolve until vue-intl locale data is fully loaded. This maintains the contract that i18n setup is complete before the Promise resolves.Also applies to: 273-273
Build Artifacts
|
This commit implements three high-priority fixes identified in the
webpack bundle analysis that will reduce total bundle size by an
estimated 2.75-4.75 MB (13-23% reduction).
- File: packages/kolibri-build/src/webpack.config.base.js
- Change: mangle: false → mangle: { safari10: true }
- Expected savings: 2-4 MB across all bundles
Terser mangling was disabled, preventing variable name shortening
which typically provides 30-40% size reduction. This single-line
change enables it with Safari 10 compatibility.
- File: packages/kolibri/composables/useUser.js
- Change: import { pick } from 'lodash' → import pick from 'lodash/pick'
- Expected savings: ~450 KB in core bundle
A single bad import was causing webpack to bundle the entire lodash
library (459 KB) instead of just the pick function (~5 KB).
packages/kolibri/utils/i18n.js (consumer)
packages/kolibri/utils/internal/vue-intl-locale-data.js (generated)
- Expected savings: ~300 KB
Root cause: The intl polyfill locale data was correctly lazy-loaded,
but vue-intl locale data was not. The code generator was producing
synchronous require() for all 30+ vue-intl locales, bundling them
in the main chunk (336 KB total).
Fix: Updated code generators to use import() pattern and updated consumption code to handle async loading. Now only
the current language's vue-intl locale data is loaded.
Defer registration of nav components which can now happen before vue intl setup. This was always a problem for
consumers of the intl polyfill, but we had mostly not noticed it until now.
Updated kolibri-sandbox to use Babel's useBuiltIns: 'usage' instead of explicit core-js imports. This eliminates ~330 KB of duplication across bloompub_viewer, html5_viewer, and learn.app bundles. Changes: - Set babel useBuiltIns to 'usage' with corejs 3.46 - Removed explicit core-js imports from iframe.js and xAPI files - Babel now auto-injects polyfills only during standalone build - When imported into main bundles, no core-js duplication occurs Expected savings: ~330 KB across 3 viewer bundles
rtibbles
force-pushed
the
claude/analyze-webpack-build-01KdiNGodpTcF1ipRfguPxwF
branch
from
e3b72a7 to
5870e8d
Compare
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
One potentially suspect change in the kolibri-sandbox webpack configuration.
| parallel: true, | ||
| terserOptions: { | ||
| mangle: false, | ||
| mangle: { |
There was a problem hiding this comment.
I can't precisely see why we've never enabled this option, but this is responsible for the majority of the savings here.
For context, this is where this was introduced (when we switched from one uglifier to Terser): 980aa2a
| `; | ||
| const vueIntlHeader = `module.exports = function () { | ||
| const data = [];`; | ||
| const vueIntlHeader = `module.exports = function (locale) { |
There was a problem hiding this comment.
I am not entirely sure why we never dynamically loaded these polyfills previously - I think because one of them was always needed, so we just bundled all of them.
Another alternative would be to add loading of these to the base HTML page, so that we don't end up with deferred loading, but I doubt it would make much difference timing wise.
|
|
||
| return ` | ||
| case '${language.intl_code}': | ||
| return new Promise(function(resolve) { |
There was a problem hiding this comment.
This code was originally written before our webpack version supported import() statements, so just updating this as well while we're here.
| }; | ||
| /* eslint-enable vue/one-component-per-file */ | ||
|
|
||
| module.exports = async function () { |
There was a problem hiding this comment.
Jest allows you to export an async function as the module.exports of the setup module, and it will wait for it to complete before starting any tests. This is needed as otherwise tests start before the i18n configuration has finished.
Previously this wasn't a problem because the vue i18n data was loaded synchronously, but with the changes above, this is now necessary.
| * xAPI Constants | ||
| */ | ||
|
|
||
| import 'core-js/features/set'; |
There was a problem hiding this comment.
We were individually polyfilling specific features in the kolibri-sandbox code. Much better to use babel present env to handle this for us instead.
|
|
||
| export const registerNavItem = component => { | ||
| if (!navItems.includes(component)) { | ||
| if (!i18nReady.value) { |
There was a problem hiding this comment.
All the changes in this file were necessitated by the fact that the nav item registration process validates that the nav items have a label - which is invariably a translated string. All the changes here do is wait for i18nReady to be true, and then do the validation.
An alternative here would be to avoid the complexity and instead change the validation to avoid the string being evaluated early.
| import store from 'kolibri/store'; | ||
| import { LoginErrors, ERROR_CONSTANTS, UPDATE_MODAL_DISMISSED, UserKinds } from 'kolibri/constants'; | ||
| import { pick } from 'lodash'; | ||
| import pick from 'lodash/pick'; |
There was a problem hiding this comment.
This is such a big gotcha, I wonder if we should make a linting rule for it - but because we're not using the ESM version of lodash, the previous code here ends up importing the entirety of lodash, not just the pick function.
| module.exports = function (locale) { | ||
| switch (locale) { | ||
| case 'ar': | ||
| return new Promise(function (resolve) { |
There was a problem hiding this comment.
These changes are just a result of the change in automated code generation.
| * | ||
| * Polyfill files are copied to ./polyfills/ directory to avoid external dependencies. | ||
| */ | ||
| module.exports = function () { |
There was a problem hiding this comment.
These changes are just a result of the change in automated code generation.
| reject(); | ||
| }, | ||
| ); | ||
| if (Object.prototype.hasOwnProperty.call(global, 'Intl') || skipPolyfill) { |
There was a problem hiding this comment.
Now seemed as good a time as any to change this function to async/await to avoid it being so verbose.
Summary
Reviewer guidance
The build should still work as expected, and it will be worth testing this on Safari to ensure the polyfilling behaviour is still intact.
Manual testing of HTML5, H5P, and Bloompub resources would also be useful.
🤖 This was created by Claude Code. @rtibbles then reviewed the generated output, and did iterative rounds of updates before making it ready for review 🤖