-
Notifications
You must be signed in to change notification settings - Fork 852
Reduce JS production build size by ~25% #13912
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
base: release-v0.19.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ const path = require('path'); | |
| const fs = require('fs'); | ||
| const { writeSourceToFile } = require('kolibri-format'); | ||
| const logger = require('kolibri-logging'); | ||
| const uniqBy = require('lodash/uniqBy'); | ||
|
|
||
| const logging = logger.getLogger('Kolibri Intl Data'); | ||
|
|
||
|
|
@@ -49,13 +50,15 @@ module.exports = function (outputDir, languageInfoPath) { | |
| * Polyfill files are copied to ./polyfills/ directory to avoid external dependencies. | ||
| */ | ||
| `; | ||
| const vueIntlHeader = `module.exports = function () { | ||
| const data = [];`; | ||
| const vueIntlHeader = `module.exports = function (locale) { | ||
| switch (locale) {`; | ||
marcellamaki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| function generateVueIntlItems(language) { | ||
| /* | ||
| * Generate entries of this form: | ||
| * data.push(require('vue-intl/locale-data/ar.js')); | ||
| * Generate entries of this form with lazy loading: | ||
| * | ||
| * case 'ar': | ||
| * return import('vue-intl/locale-data/ar.js'); | ||
| * | ||
| * Some Intl codes look like 'ar' and others look like 'bn-bd', so for Vue Intl | ||
| * we strip off the territory code if it's there. | ||
|
|
@@ -88,17 +91,26 @@ module.exports = function (outputDir, languageInfoPath) { | |
| } | ||
| } | ||
|
|
||
| return `data.push(require('${module_path}'));`; | ||
| return ` | ||
| case '${vue_intl_code}': | ||
| return import('${module_path}');`; | ||
| } | ||
| } | ||
|
|
||
| const vueIntlFooter = ` | ||
| return data; | ||
| default: | ||
| return import('vue-intl/locale-data/en.js'); | ||
| } | ||
| }; | ||
| `; | ||
|
|
||
| const vueIntlModule = | ||
| commonHeader + vueIntlHeader + languageInfo.map(generateVueIntlItems).join('') + vueIntlFooter; | ||
| commonHeader + | ||
| vueIntlHeader + | ||
| uniqBy(languageInfo, l => l.intl_code.split('-')[0]) | ||
| .map(generateVueIntlItems) | ||
| .join('') + | ||
| vueIntlFooter; | ||
|
|
||
| const vueIntlModulePath = path.resolve(outputDir, 'vue-intl-locale-data.js'); | ||
| const intlHeader = `module.exports = function(locale) { | ||
|
|
@@ -109,14 +121,7 @@ module.exports = function (outputDir, languageInfoPath) { | |
| * Generate entries of the form: | ||
| * | ||
| * case 'sw-tz': | ||
| * return new Promise(function(resolve) { | ||
| * require.ensure( | ||
| * ['intl/locale-data/jsonp/sw-TZ.js'], | ||
| * function(require) { | ||
| * resolve(() => require('intl/locale-data/jsonp/sw-TZ.js')); | ||
| * } | ||
| * ); | ||
| * }); | ||
| * return import('intl/locale-data/jsonp/sw-TZ.js'); | ||
| * | ||
| * Note that not all codes have two parts, e.g. 'en' vs 'es-mx'. | ||
| */ | ||
|
|
@@ -174,27 +179,13 @@ module.exports = function (outputDir, languageInfoPath) { | |
|
|
||
| return ` | ||
| case '${language.intl_code}': | ||
| return new Promise(function(resolve) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was originally written before our webpack version supported |
||
| require.ensure( | ||
| ['${module_path}'], | ||
| function(require) { | ||
| resolve(() => require('${module_path}')); | ||
| } | ||
| ); | ||
| });`; | ||
| return import('${module_path}');`; | ||
| } | ||
| } | ||
|
|
||
| const intlFooter = ` | ||
| default: | ||
| return new Promise(function(resolve) { | ||
| require.ensure( | ||
| ['intl/locale-data/jsonp/en.js'], | ||
| function(require) { | ||
| resolve(() => require('intl/locale-data/jsonp/en.js')); | ||
| } | ||
| ); | ||
| }); | ||
| return import('intl/locale-data/jsonp/en.js'); | ||
| } | ||
| }; | ||
| `; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,8 +56,6 @@ Vue.config.silent = true; | |
| Vue.config.devtools = false; | ||
| Vue.config.productionTip = false; | ||
|
|
||
| i18nSetup(true); | ||
|
|
||
| Object.defineProperty(window, 'scrollTo', { value: () => {}, writable: true }); | ||
|
|
||
| // Shows better NodeJS unhandled promise rejection errors | ||
|
|
@@ -76,3 +74,7 @@ global.flushPromises = function flushPromises() { | |
| }); | ||
| }; | ||
| /* eslint-enable vue/one-component-per-file */ | ||
|
|
||
| module.exports = async function () { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| await i18nSetup(true); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,8 @@ module.exports = { | |
| [ | ||
| '@babel/preset-env', | ||
| { | ||
| useBuiltIns: false, | ||
| useBuiltIns: 'usage', | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This configuration forces corejs to polyfill things as they are used. We avoid this in the main Kolibri webpack configuration, and instead just provide a blanket environment polyfill with corejs, so as to avoid repeating polyfills and shims across plugins. |
||
| corejs: '3.46', | ||
| }, | ||
| ], | ||
| ], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,6 @@ | |
| * xAPI Constants | ||
| */ | ||
|
|
||
| import 'core-js/features/set'; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 OBJECT_TYPES = { | ||
| AGENT: 'Agent', | ||
| GROUP: 'Group', | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really do not know how to read/review this file effectively
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross check it against the webpack configurations in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i checked against 9.1 and 10.1 (which were available in browserstack) and ran the pex locally, for both i got a browser not supported message which matches with what I see in the browser list ('Safari >= 11.1') - but just wanted to confirm that this is also what you were thinking would happen.... |
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.
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.