Fixes #24570 - [JS] add dynamic import #5934

amirfefer · 2018-08-08T15:25:56Z

some modules (such as brace, password-strength...) takes a large cut from the bundle/vendor file.
this solution imports these modules dynamically on demand with react-loadable

Production vendor file without dynamic import

Production vendor file with dynamic import (password-strength only)

theforeman-bot · 2018-08-08T15:26:08Z

Issues: #24570

tbrisker · 2018-08-09T08:40:03Z

I'm concerned this will cause parts of libraries that aren't used by core but needed for plugins to be missing from vendor.js and cause plugins to break.
Also, won't this cause loads to be slow for missing chunks while navigating? I thought we agreed it's better to have one large file downloaded once and cached instead of many small files?

ohadlevy · 2018-08-09T08:41:03Z

config/webpack.config.js

+    publicPath = process.env.ASSET_PATH || '/webpack/';
+  } else {
+    publicPath = process.env.ASSET_PATH || `http://${devServerHostname}:${devServerPort}/webpack/`;
+  }


nitpick, can be written as:

publicPath = process.env.ASSET_PATH || (production ? '/webpack/' : `http://${devServerHostname}:${devServerPort}/webpack/`);

but why is it needed? (or maybe it should be a separate PR?)

it needed for webpackChunkName, otherwise on develop env the splitted js won't be reachable.

Yeah, I didn't find any better way for that than what Amir has here.
I wonder if we can somehow guess http vs https from config. That would make it smoother. But I'm fine with the code as is if we can't find it.

@amirfefer I found it! We can use process.argv.includes('--https')

ohadlevy · 2018-08-09T08:41:29Z

package.json

@@ -12,10 +12,12 @@
    "axios-mock-adapter": "^1.10.0",
    "babel-cli": "^6.10.1",
    "babel-core": "^6.26.3",
-    "babel-eslint": "^6.1.2",
+    "babel-eslint": "^7.2.3",


is this required for this PR?

ohadlevy · 2018-08-09T08:42:12Z

webpack/assets/javascripts/react_app/components/PasswordStrength/PasswordStrength.js

 import CommonForm from '../common/forms/CommonForm';

 import './PasswordStrength.scss';

+const LoadingComponent = () => <div> Loading... </div>;


maybe use our Loader component? at least, the spinner from it (note text is also not extracted)

ohadlevy · 2018-08-09T08:44:33Z

@amirfefer It turns out i did start writing something similar before, can you review ohadlevy@4a33ba7? thanks!

ohadlevy · 2018-08-09T08:52:23Z

I'm concerned this will cause parts of libraries that aren't used by core but needed for plugins to be missing from vendor.js and cause plugins to break.

Why? if a plugin import a component, it would simply have a webpack generated code to load it on demand, AFAIU it should be 100% comparable with current state.

Also, won't this cause loads to be slow for missing chunks while navigating? I thought we agreed it's better to have one large file downloaded once and cached instead of many small files?

I guess it depends, does it make sense to load an extra 1MB that is used in only one page? (e.g. password strength), I believe the best practice today is to split, and load on demand, sometimes with rel=prefetch to make things even faster, I think its similar to how we load translations for only one language instead of all of them?

amirfefer · 2018-08-09T11:03:41Z

I'm concerned this will cause parts of libraries that aren't used by core but needed for plugins to be missing from vendor.js and cause plugins to break.

I've tested password-strength over katello, and it works well, even though it doesn't exist in vendor.js

Also, won't this cause loads to be slow for missing chunks while navigating?

we can use Prefetch, or other optimization webpack suggests.

I thought we agreed it's better to have one large file downloaded once and cached instead of many small files?

as long as the javascript stack becomes bigger - then it would be better to use code splitting as it recommended by webpack.

ohadlevy · 2018-08-09T11:48:57Z

I've tested password-strength over katello, and it works well, even though it doesn't exist in vendor.js

of course it doesn't exist in vendor, its not in its own file? good to know Katello works as expected.

tbrisker · 2018-08-09T11:49:04Z

@amirfefer did you test it with an RPM build or just in development? following the issues we've had i'd be very worried about the vendor.js file not including some code plugins expect it to include.

I'm not really worried about a few hundred KBs in download size. Most of our users are connected over fast local ethernet, so there's no big difference in time, and anyways it should only be downloaded once and cached afterwards, so I don't see how this benefits us. I expect if they have to wait for another download when, in this example, editing a user - it would give worse user experience. We've had this discussion before (when we were talking about page-specific js code), and we decided to try and push everything into application.js and remove the page-specific files. I'm wondering what changed.

Keep in mind that webpack's recommended practices are written with SaaS in mind. In that case, you need to provide resources over sometimes slow connections, reduce network costs, and have complete control of their stack - non of which is true for foreman.

ohadlevy · 2018-08-09T11:52:23Z

config/webpack.config.js

@@ -11,10 +11,21 @@ var LodashModuleReplacementPlugin = require('lodash-webpack-plugin');
 var pluginUtils = require('../script/plugin_webpack_directories');
 var vendorEntry = require('./webpack.vendor');
 var SimpleNamedModulesPlugin = require('../webpack/simple_named_modules');
+var { execSync } = require('child_process');
+
+ const getHostname = () => {


I would suggest to call the argument bindhost instead of hostname (to alignt to rails, webpack etc).
also, in order to get the hostname in node, please use something like

var os = require('os'); const devServerHostname = process.env.HOSTNAME || os.hostname();

amirfefer · 2018-08-09T12:32:12Z

@tbrisker
Tested on compiled production (w/o webpack devserver)

tbrisker · 2018-08-09T12:38:44Z

@amirfefer did you compile vendor.js separately without katello, then used that with katello that built its own bundle when testing? Or did you just compile all assets together?

ekohl · 2018-08-10T08:39:58Z

config/webpack.config.js

+  if (production) {
+    publicPath = process.env.ASSET_PATH || '/webpack/';
+  } else {
+    publicPath = process.env.ASSET_PATH || `http://${devServerHostname}:${devServerPort}/webpack/`;


Note that in the Katello development setup we run with https

waldenraines · 2018-08-14T18:38:52Z

I just want to agree with @amirfefer and @ohadlevy on this (sorry @tbrisker 😄). I think that this is a good change as long as plugins also have a way to require the necessary libraries and we document that this needs to be done.

tstrachota · 2018-08-26T08:08:04Z

I agree with moving less frequently used code to separate chunks. Prefetch should cover concerns about certain pages being slowed down.
We'll need dynamic imports in future to support translations anyway.

tbrisker

I don't feel like my concerns were properly addressed.

No one has confirmed this won't break plugins on packaged installs. Plugins may expect certain libraries to be available in the vendor.js file, which this will remove (right now just pw strength, but if i understand correctly the aim is to have multiple such libraries). Considering the amount of issues we've had around this exact area so far, i'd be very wary of introducing changes that aren't properly tested on a packaged build.

Additionally, no one has explained why we decided to change direction from trying to load all JS in one big chunk once to loading additional files on certain pages, which will require re-rendering those pages once the new JS is loaded leading to a feeling of slower load times. In fact, the argument was made that we should move to SPA (among other reasons) in order to reduce the number of server round-trips needed before a page can be properly displayed.

I don't think saving a few hundred kb's in the production assets size, which is downloaded once over a usually very fast connection, is worth the risk and tradeoffs here, and haven't been convinced yet of what value this actually brings other than a bit smaller vendor bundle. So, all in all, for me this is currently a 👎 .

amirfefer · 2018-11-05T12:21:51Z

closing this for now,
I'll open a discussion at discourse later on

Fixes #24570 - [JS] add dynamic import

bef4e38

theforeman-bot added Needs testing Not yet reviewed labels Aug 8, 2018

ohadlevy reviewed Aug 9, 2018

View reviewed changes

ohadlevy requested changes Aug 9, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Aug 9, 2018

ekohl requested changes Aug 10, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Aug 10, 2018

tbrisker requested changes Aug 26, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Aug 26, 2018

amirfefer closed this Nov 5, 2018

ohadlevy mentioned this pull request Apr 29, 2019

fixes #24570 - dynamicailly import password strength component #6719

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #24570 - [JS] add dynamic import #5934

Fixes #24570 - [JS] add dynamic import #5934

amirfefer commented Aug 8, 2018 •

edited

theforeman-bot commented Aug 8, 2018

tbrisker commented Aug 9, 2018

ohadlevy Aug 9, 2018

amirfefer Aug 9, 2018

tstrachota Aug 26, 2018

tstrachota Aug 26, 2018

ohadlevy Aug 9, 2018

ohadlevy Aug 9, 2018

ohadlevy commented Aug 9, 2018

ohadlevy commented Aug 9, 2018

amirfefer commented Aug 9, 2018 •

edited

ohadlevy commented Aug 9, 2018

tbrisker commented Aug 9, 2018

ohadlevy Aug 9, 2018

amirfefer commented Aug 9, 2018

tbrisker commented Aug 9, 2018

ekohl Aug 10, 2018

waldenraines commented Aug 14, 2018 •

edited

tstrachota commented Aug 26, 2018

tbrisker left a comment

amirfefer commented Nov 5, 2018

Fixes #24570 - [JS] add dynamic import #5934

Fixes #24570 - [JS] add dynamic import #5934

Conversation

amirfefer commented Aug 8, 2018 • edited

theforeman-bot commented Aug 8, 2018

tbrisker commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Aug 9, 2018

ohadlevy commented Aug 9, 2018

amirfefer commented Aug 9, 2018 • edited

ohadlevy commented Aug 9, 2018

tbrisker commented Aug 9, 2018

Choose a reason for hiding this comment

amirfefer commented Aug 9, 2018

tbrisker commented Aug 9, 2018

Choose a reason for hiding this comment

waldenraines commented Aug 14, 2018 • edited

tstrachota commented Aug 26, 2018

tbrisker left a comment

Choose a reason for hiding this comment

amirfefer commented Nov 5, 2018

amirfefer commented Aug 8, 2018 •

edited

amirfefer commented Aug 9, 2018 •

edited

waldenraines commented Aug 14, 2018 •

edited