Move things around, make the navigation calculation run in seperate thread #955

karelhala · 2020-09-09T10:23:56Z

Features

Option to have an array of permission functions
Reduces chrome bundle size from 3.2 MB to 1.6 MB

Depracated

disabled_on_stable - this field should no longer be used as consumers can use permissions function
appNav - no longer possible to pass custom application navigation

codecov-commenter · 2020-09-09T15:39:11Z

Codecov Report

Merging #955 into master will decrease coverage by 1.45%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #955      +/-   ##
==========================================
- Coverage   52.58%   51.12%   -1.46%     
==========================================
  Files          53       56       +3     
  Lines        1046     1021      -25     
  Branches      212      200      -12     
==========================================
- Hits          550      522      -28     
- Misses        395      396       +1     
- Partials      101      103       +2

Impacted Files	Coverage Δ
src/js/App/Sidenav/AppSwitcher.js	`81.81% <ø> (ø)`
src/js/App/Sidenav/index.js	`0.00% <0.00%> (-83.34%)`	⬇️
src/js/chrome.js	`0.00% <0.00%> (ø)`
src/js/inventory/index.js	`9.09% <0.00%> (ø)`
src/js/redux/action-types.js	`100.00% <ø> (ø)`
src/js/redux/actions.js	`76.00% <ø> (-2.79%)`	⬇️
src/js/entry.js	`25.75% <17.64%> (+5.75%)`	⬆️
src/js/App/Sidenav/SideNav.js	`23.07% <23.07%> (ø)`
src/js/App/Header/index.js	`28.57% <28.57%> (-71.43%)`	⬇️
src/js/App/Sidenav/Loader.js	`33.33% <33.33%> (ø)`
... and 10 more

ryelo · 2020-09-10T16:15:30Z

src/js/App/Sidenav/Navigation.js

@@ -12,172 +12,126 @@ import './Navigation.scss';

 const basepath = document.baseURI;

-const openshiftLinks = {
-    supportcases: {
+const extraLinks = {


Can we make sure the Documentation link is always the last one?

Sure thing! I've made it to match what was here previously, but I've seen the ticket about matching position of all docs link everywhere.

Awesome. That got merged in with #942 so I just wanted to make sure we didn't revert what we just committed 😄

Hyperkid123

There is a definite improvement. I took a look at the react profiler and these are some of the observations.

Before

We have 3 renders on the initial load. The final render is committed around 3s. The number will never be precise.

After

We now have 4 renders but committed at around 1.6s so the cut on initial render time about 50% on my machine. 🎉

The biggest "bottleneck" I see is the transition from loaders to the actual navigation. That is mostly caused by evaluating the YAML file which seems to be quite an expensive operation. The jump between commits is 0.4s to 1.6s and that is with cached main.yml file.

@karelhala could we just store the nav JSON structure to sessionStorage and give it a couple of hours of timeout? Or use the same cache strategy as for the YAML file? This would further decrease the time to first interactive.

Edit: here is a gif where you can see the time commits. As you can see, it only takes around 20ms to render the whole nav from the JSON. But it takes around 1.2s to actually evaluate the code itself.

Hyperkid123 · 2020-09-11T06:26:23Z

src/js/App/Header/Loader.js

+
+const HeaderLoader = () => <React.Fragment />;
+
+export default HeaderLoader;


You can just export React.Fragment. No need for additional node in the VirtualDOM.

I wanted to use some fancy loader similiar to navigation, but had hard time doing it. Let's nuke this file and use just Fragment there.

Hyperkid123 · 2020-09-11T06:30:22Z

src/js/App/Sidenav/SideNav.js

+SideNav.propTypes = {
+    activeTechnology: PropTypes.string,
+    globalNav: PropTypes.arrayOf(PropTypes.shape({
+        [PropTypes.string]: PropTypes.any


I think you should list all used attributes instead of creating a generic object. If you want that just use PropTypes.object.

Hyperkid123 · 2020-09-11T06:32:47Z

src/js/nav/globalNav.js

@@ -10,24 +13,24 @@ export let getNavFromConfig = async (masterConfig) => {
    }, {});
 };

+const isCurrVisible = (permissions) => Promise.all(
+    flatMap([permissions], ({ method } = {}) => (
+        // (null, undefined, true) !== false


leftover comment?

Not really, this is to show what is actually evalueted here because the boolean logic is slightly more complex than it could be for new commers.

Hyperkid123 · 2020-09-11T06:34:50Z

src/js/nav/globalNav.js

+const isCurrVisible = (permissions) => Promise.all(
+    flatMap([permissions], ({ method } = {}) => (
+        // (null, undefined, true) !== false
+        visibilityFunctions?.[method]?.(...permissions.args || []) !== false || false


I don't think the || false is necessary here. x !== false will always return boolean value. Or am I crazy 😆 I must say this makes me think much harder than it should.

Yeah, the false is not necessary here, the logic is if visibility function return something else then false return true, otherwise false. But that's correct and doesn't need the || false

Hyperkid123 · 2020-09-11T06:36:18Z

src/js/nav/globalNav.js

+    const groupedNav = await getNavFromConfig(safeLoad(yamlConfig));
+
+    const [active, section] = [getUrl('bundle'), getUrl('app')];
+    const globalNav = (groupedNav[active] || groupedNav.insights).routes;


This has the potential to crash. I would add ?.

Hyperkid123 · 2020-09-11T06:36:43Z

src/js/nav/globalNav.js

+
+    const [active, section] = [getUrl('bundle'), getUrl('app')];
+    const globalNav = (groupedNav[active] || groupedNav.insights).routes;
+    let activeSection = globalNav.find(({ id }) => id === section);


activeSection is never re-assigned.

karelhala · 2020-09-11T08:13:41Z

@Hyperkid123 I was thinking about storing the calculated JSON in cache as well, but didn't wanted to bloat this PR even more. But let's do it to improve loads even more!

Hyperkid123 · 2020-09-11T08:25:57Z

@Hyperkid123 I was thinking about storing the calculated JSON in cache as well, but didn't wanted to bloat this PR even more. But let's do it to improve loads even more!

@karelhala We can do it in different PR if you want. It will make it easier to review and test.

…hread

Hyperkid123 · 2020-09-14T08:33:11Z

src/js/App/Sidenav/__snapshots__/Navigation.test.js.snap

@@ -244,7 +244,7 @@ exports[`Navigation should render corectly 1`] = `
    ariaRightScroll="Scroll right"
  >
    <NavExpandable
-      className="ins-m-navigation-align"


This css is required to fix the navigation text left alignment. Kan you keep it? (introduced in #958)

Sure thing!

karelhala force-pushed the refactor-navigation branch 3 times, most recently from 5f2323f to e3f0e48 Compare September 9, 2020 15:34

karelhala force-pushed the refactor-navigation branch 4 times, most recently from 15a35d0 to fc26489 Compare September 10, 2020 14:28

karelhala marked this pull request as ready for review September 10, 2020 14:28

karelhala requested review from ryelo, Hyperkid123, PanSpagetka and fhlavac September 10, 2020 14:29

ryelo reviewed Sep 10, 2020

View reviewed changes

Hyperkid123 reviewed Sep 11, 2020

View reviewed changes

karelhala force-pushed the refactor-navigation branch 2 times, most recently from 3b50bf9 to c4dc7b6 Compare September 11, 2020 13:41

Hyperkid123 previously approved these changes Sep 14, 2020

View reviewed changes

karelhala added 5 commits September 14, 2020 10:11

Move things around, make the navigation calculation run in seperate t…

e5816f8

…hread

Simplify navigation and header by introducing loader functions

4e14ca3

Update logic for navigation

a83a0c6

Simplify navigation component

3870eba

Refactor how permissions are calculated

581386a

karelhala dismissed Hyperkid123’s stale review via 1967bfd September 14, 2020 08:12

karelhala force-pushed the refactor-navigation branch 2 times, most recently from 1967bfd to bf437b5 Compare September 14, 2020 08:25

Hyperkid123 reviewed Sep 14, 2020

View reviewed changes

Update loaders of navigation

9b18f26

karelhala force-pushed the refactor-navigation branch from bf437b5 to 9b18f26 Compare September 14, 2020 11:04

Hyperkid123 approved these changes Sep 14, 2020

View reviewed changes

karelhala merged commit b26241a into RedHatInsights:master Sep 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move things around, make the navigation calculation run in seperate thread #955

Move things around, make the navigation calculation run in seperate thread #955

karelhala commented Sep 9, 2020 •

edited

Loading

codecov-commenter commented Sep 9, 2020 •

edited

Loading

ryelo Sep 10, 2020

karelhala Sep 10, 2020

ryelo Sep 10, 2020

Hyperkid123 left a comment •

edited

Loading

Hyperkid123 Sep 11, 2020

karelhala Sep 11, 2020

Hyperkid123 Sep 11, 2020

Hyperkid123 Sep 11, 2020

karelhala Sep 11, 2020

Hyperkid123 Sep 11, 2020

karelhala Sep 11, 2020

Hyperkid123 Sep 11, 2020

Hyperkid123 Sep 11, 2020

karelhala commented Sep 11, 2020

Hyperkid123 commented Sep 11, 2020 •

edited

Loading

Hyperkid123 Sep 14, 2020

karelhala Sep 14, 2020


		const HeaderLoader = () => <React.Fragment />;

		export default HeaderLoader;

Move things around, make the navigation calculation run in seperate thread #955

Move things around, make the navigation calculation run in seperate thread #955

Conversation

karelhala commented Sep 9, 2020 • edited Loading

Features

Depracated

codecov-commenter commented Sep 9, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 left a comment • edited Loading

Choose a reason for hiding this comment

Before

After

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

karelhala commented Sep 11, 2020

Hyperkid123 commented Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelhala commented Sep 9, 2020 •

edited

Loading

codecov-commenter commented Sep 9, 2020 •

edited

Loading

Hyperkid123 left a comment •

edited

Loading

Hyperkid123 commented Sep 11, 2020 •

edited

Loading