fixes #24570 - dynamicailly import password strength component #6719

ohadlevy · 2019-04-29T21:01:22Z

this reduced the vendor file by about 800kb and reduce
every full page reload by about 100ms on my machine

theforeman-bot · 2019-04-29T21:01:39Z

Issues: #24570

ohadlevy · 2019-04-29T21:04:19Z

webpack analyze before

webpack analyze after

vendor file size decreased by about 800K
(not to mention that it should not be exposed to plugins to begin with imho)

and my chrome loading statistics shows a decrease in script evaluation for about 85ms +-

ohadlevy · 2019-04-29T21:04:48Z

this of course based on @amirfefer work in #5934

ares · 2019-04-30T06:04:51Z

Could you please help me understand how the loading changed? Is this loading the component somehow via async request only when needed?

Edit: also was the original concern from @tbrisker from the original PR addressed? I may have missed some discourse discussion.

ohadlevy · 2019-04-30T06:09:10Z

Could you please help me understand how the loading changed? Is this loading the component somehow via async request only when needed?

yes, this loads the the component async on demand, e.g. only when the react component is mounted, it would make an additional script call to fetch the code itself, this imho reduce the time the browser needs to parse the content (in this case about 800kb of common password strings) and delay any page load because of it, it is very similar to adding an additional JavaScript tag in a view but without the penalty of turoblink full page reloads.

if this approach is supported, I plan to continue and move other non common code paths (such as novnc, spice, ace editor etc) to separate bundles to continuously improve initial / full page (re)loads.

ohadlevy · 2019-04-30T06:10:06Z

I forgot to mention this also uses the patternfly loaderState component, that only shows a spinner if its loading over 250ms (or something like that),

theforeman-bot · 2019-04-30T06:50:16Z

There were the following issues with the commit message:

50bdc2f must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

ohadlevy · 2019-04-30T06:51:12Z

as an example, I've moved novnc to its own bundle as well, I'm not fixing the current test failures as I would like to get an overall ack/nack on the path.

amirfefer

every full page reload by about 100ms on my machine

verified, ~100ms doesn't sound much, but if we will choose this approach, over time the differences would be more significant.

ekohl · 2019-04-30T11:44:14Z

Not going to comment on the implementation, but the general direction sounds correct to me. Will this be an issue for plugins or is it fully transparent for them?

ohadlevy · 2019-04-30T11:55:17Z

Not going to comment on the implementation, but the general direction sounds correct to me.

I'm looking for input from @sharvit and @amirfefer etc...

Will this be an issue for plugins or is it fully transparent for them?

I don't expect any issues.

sharvit

Thanks for bringing it up @ohadlevy, removing the password-strength and moving it into a separate bundle that loads on demand makes sense to me and I believe it's a good direction.

Hasn't tested it yet but the implementation using react-loadable with patternfly-react LoadingState looks good and simple for me 👍

sharvit · 2019-04-30T12:30:20Z

config/webpack.vendor.js

@@ -68,5 +67,5 @@ module.exports = [
  'jstz',
  'urijs',
  'uuid',
-  '@novnc/novnc/core/rfb.js',
+  // '@novnc/novnc/core/rfb.js',


Does it relate to the PR scope?

it should be removed.... @sharvit can you answer the following:

where should i put the loading section? I put it in index next to connect, not sure if we should split the two?

how do you suggest to do testings? have a look at the current failure (which I think might impact your answer on the first question)

thanks!

Before you go ahead too far, I'd like to hear from @tbrisker too. I've seen clear -1 from him previously. Chances are that concerns were addressed or no longer apply.

@amirfefer, was the 100ms boost in dev or production environment? If in dev, can you also check impact on production?

where should i put the loading section? I put it in index next to connect, not sure if we should split the two?

how do you suggest to do testings? have a look at the current failure (which I think might impact your answer on the first question)

I believe it belongs to the index.js while the integration.test.js should test the different loading states (pending, error, success).
Does the error appear because the integration.test.js doesn't wait for the loading?

Another direction can be to wrap the react-password-strength lib with Loadable

@sharvit I did try both (in index and to wrap the npm package itself) in both cases the PasswordStrength/tests/integration.test.js should flow test failed

I didn't get a chance to look into this PR too much, but IIRC my main concern in the past was around the change being focused on reducing download size, which is a one-time cost on production environments as all the assets should be cached and networks tend to be fast.
If this leads to a measurable improvement in page rendering speed in production than i don't have objections - as long as we have some benchmark results showing the improvement to be significant enough to be worth the extra effort and complexity.
We should benchmark the difference both on pages that require the dynamically imported components and those that don't - I'd expect faster loads on pages that don't and slower results on pages that do (since they would need to wait for the extra file to be loaded and rendered before completing their own render), at which point we can say "we would rather wait 200ms more on pages X to make pages !X faster by 100ms"

@sharvit I did try both (in index and to wrap the npm package itself) in both cases the PasswordStrength/tests/integration.test.js should flow test failed

I believe it happens because of the integration.test.js doesn’t wait for the loading promise to get resolved.

theforeman-bot · 2019-05-01T12:23:57Z

There were the following issues with the commit message:

2edd8fa must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

ohadlevy · 2019-05-26T14:26:08Z

OK, I've a few updates for this thread, I've

Added Spice
Added Editor (which also bundle patternfly-react-extensions)

I've done the following performance tests

tl;dr

	Current	Proposed	Difference (ms)
First Load	1677	1127	550
Average Page Load	711.4	641.8	69.6

Memory size impact is about half on initial page loads

Bundle size impact

	Current	Proposed
vendor	4.57 MB	2.31 MB
bundle	896.66 KB	488.99 KB
spice		141.53 KB
noVNC		138.6 KB
Editor		1.57 MB
passwordStrength		822.92 KB

I've done a total of 7 tests for each environment (as its done manually via chrome performance developers tools, raw data at

Current	ms	msLoading	msScripting	msRendering	msPainting	msSystem	msIdle
First Load	1677	49.3	1259.4	34.7	1.1	191.6	141.2
2	650	36.1	406.1	17	0.6	132.5	58.2
3	640	33.4	390.7	21.3	0.6	135.7	58.6
4	767	37.1	447.7	21.8	5.4	174.1	80.9
5	705	38	365.7	20.4	0.9	213.6	66.7
6	795	30.1	378.4	16	0.7	296.7	72.9
Proposed
First Load	1127	32.7	870.5	46.4	0.9	73	103.2
2	538	28.9	315.5	16.9	0.6	87.4	88.5
3	770	8.1	291.4	14.3	0.9	118.9	336.3
4	485	11.7	297.6	15	0.6	90.6	69.7
5	653	6.3	290.9	15.1	0.7	109	230.7
6	763	8.1	298.1	16.7	0.8	101.3	338.2

If you would like to try this out, simply change your docker-compose.yml file

diff --git a/docker-compose.yml b/docker-compose.yml
index 821984ff2..16d67676e 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -20,7 +20,8 @@ services:
       - db:/var/lib/mysql/data
 
   app: &app_base
-    image: quay.io/foreman/foreman:develop
+    image: quay.io/ohadlevy/foreman:react-loadable
     command: bundle exec bin/rails server -b 0.0.0.0
     build:
       context: .

ohadlevy · 2019-05-26T14:34:00Z

it also goes without saying that I was testing this on a modern machine, with zero latency, I would suspect the performance impact would be bigger on slower devices (older hardware, mobile/tables etc).

ohadlevy · 2019-05-28T14:44:17Z

@glekner can you have a look if you can find a way to get the integration tests working?

ohadlevy · 2019-06-04T14:19:54Z

/cc @sharvit @amirfefer

this reduced the vendor file by about 800kb and reduce every full page reload by about 100ms on my machine added novnc to its own bundle

ohadlevy · 2019-06-10T19:24:30Z

@sharvit https://stackoverflow.com/questions/50176706/how-test-a-react-loadable-component has some options, what do you think?

theforeman-bot · 2019-09-18T08:22:10Z

@ohadlevy, this pull request is currently not mergeable. Please rebase against the develop branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream develop

This message was auto-generated by Foreman's prprocessor

sharvit · 2019-09-23T05:32:55Z

I am sorry for the inconvenient @ohadlevy but this PR is not mergeable because it should be implemented in the vendor now.

Adding it to the foreman-js roadmap.md as it should be there:
theforeman/foreman-js#50

theforeman-bot added Needs testing Not yet reviewed UI labels Apr 29, 2019

theforeman-bot added the Waiting on contributor label Apr 30, 2019

amirfefer reviewed Apr 30, 2019

View reviewed changes

sharvit reviewed Apr 30, 2019

View reviewed changes

ohadlevy force-pushed the react-loadable branch from 50bdc2f to 2edd8fa Compare May 1, 2019 12:23

theforeman-bot removed the Waiting on contributor label May 1, 2019

theforeman-bot added the Waiting on contributor label May 1, 2019

ohadlevy force-pushed the react-loadable branch from 2edd8fa to c01968f Compare May 26, 2019 12:46

theforeman-bot removed the Waiting on contributor label May 26, 2019

fixes #24570 - dynamicailly import password strength component

78b7587

this reduced the vendor file by about 800kb and reduce every full page reload by about 100ms on my machine added novnc to its own bundle

ohadlevy force-pushed the react-loadable branch from c01968f to 78b7587 Compare June 4, 2019 14:20

ohadlevy mentioned this pull request Jun 26, 2019

Fixes #27074 - Migrate to @theforeman/vendor and @theforeman/vendor-dev pkg #6605

Merged

tbrisker assigned xprazak2 Sep 18, 2019

theforeman-bot added Waiting on contributor and removed Needs testing Not yet reviewed labels Sep 18, 2019

ohadlevy closed this Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #24570 - dynamicailly import password strength component #6719

fixes #24570 - dynamicailly import password strength component #6719

ohadlevy commented Apr 29, 2019

theforeman-bot commented Apr 29, 2019

ohadlevy commented Apr 29, 2019

ohadlevy commented Apr 29, 2019

ares commented Apr 30, 2019 •

edited

ohadlevy commented Apr 30, 2019

ohadlevy commented Apr 30, 2019

theforeman-bot commented Apr 30, 2019

ohadlevy commented Apr 30, 2019

amirfefer left a comment •

edited

ekohl commented Apr 30, 2019

ohadlevy commented Apr 30, 2019

sharvit left a comment

sharvit Apr 30, 2019

ohadlevy Apr 30, 2019

ares Apr 30, 2019

sharvit May 1, 2019

sharvit May 1, 2019

ohadlevy May 1, 2019

tbrisker May 1, 2019

sharvit May 1, 2019

theforeman-bot commented May 1, 2019

ohadlevy commented May 26, 2019

ohadlevy commented May 26, 2019

ohadlevy commented May 28, 2019

ohadlevy commented Jun 4, 2019

ohadlevy commented Jun 10, 2019

theforeman-bot commented Sep 18, 2019

sharvit commented Sep 23, 2019

fixes #24570 - dynamicailly import password strength component #6719

fixes #24570 - dynamicailly import password strength component #6719

Conversation

ohadlevy commented Apr 29, 2019

theforeman-bot commented Apr 29, 2019

ohadlevy commented Apr 29, 2019

ohadlevy commented Apr 29, 2019

ares commented Apr 30, 2019 • edited

ohadlevy commented Apr 30, 2019

ohadlevy commented Apr 30, 2019

theforeman-bot commented Apr 30, 2019

ohadlevy commented Apr 30, 2019

amirfefer left a comment • edited

Choose a reason for hiding this comment

ekohl commented Apr 30, 2019

ohadlevy commented Apr 30, 2019

sharvit left a 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

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

theforeman-bot commented May 1, 2019

ohadlevy commented May 26, 2019

ohadlevy commented May 26, 2019

ohadlevy commented May 28, 2019

ohadlevy commented Jun 4, 2019

ohadlevy commented Jun 10, 2019

theforeman-bot commented Sep 18, 2019

sharvit commented Sep 23, 2019

ares commented Apr 30, 2019 •

edited

amirfefer left a comment •

edited