Feat/reduce bundle size #106

icyJoseph · 2020-07-07T08:50:59Z

This PR tries to implement #55 :)

It introduces a bunch of refactoring of Routes, removes React-Markdown, applies React features such as lazy loading + Suspense and Webpack dynamic imports.

Today navigating to an invalid Route, renders a table with a bunch of zeroes. This takes care of it.

I know one should squash commits, we can do that, but would be nice to discuss the PR first. The commits list helps follow how the feature evolved.

I also needed to merge master, we should make sure things are alright.

Cheers!

codecov · 2020-07-07T08:58:17Z

Codecov Report

Merging #106 into master will increase coverage by 1.80%.
The diff coverage is 85.62%.

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   76.80%   78.61%   +1.80%     
==========================================
  Files          55       38      -17     
  Lines        1341     1230     -111     
  Branches      155      141      -14     
==========================================
- Hits         1030      967      -63     
+ Misses        275      235      -40     
+ Partials       36       28       -8

Impacted Files	Coverage Δ
src/components/generic/SmallWidgets.js	`90.00% <ø> (ø)`
src/components/pages/WalkingGaitsPage.js	`0.00% <0.00%> (-9.84%)`	⬇️
src/components/pages/InverseKinematicsPage.js	`59.25% <16.66%> (+3.70%)`	⬆️
src/components/DimensionsWidget.js	`67.85% <33.33%> (+1.19%)`	⬆️
src/components/pages/LegPatternPage.js	`57.89% <33.33%> (ø)`
src/components/pages/ForwardKinematicsPage.js	`74.07% <50.00%> (+0.99%)`	⬆️
src/components/hooks/useHexapodParams.js	`84.61% <84.61%> (ø)`
src/components/hooks/usePlot.js	`84.61% <84.61%> (ø)`
src/loadables.js	`88.00% <88.00%> (ø)`
src/App.js	`100.00% <100.00%> (+14.28%)`	⬆️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f02fd4d...a51f61f. Read the comment docs.

icyJoseph · 2020-07-07T09:16:39Z

I had done a bad glob pattern, but a later commit fixes it.

mithi · 2020-07-07T10:33:01Z

For comparison

Before

After

|

mithi · 2020-07-07T10:40:34Z

src/components/pages/NoMatch.js

+
+export default () => (
+    <div className="no-match">
+        <h1>404</h1>


Really cool 404 page 😊

mithi · 2020-07-07T11:19:19Z

package.json

@@ -4,7 +4,7 @@
    "private": true,
    "dependencies": {
        "@testing-library/jest-dom": "^4.2.4",
-        "@testing-library/react": "^9.3.2",
+        "@testing-library/react": "^10.4.4",


Thanks for bumping this up 👍 💯

mithi · 2020-07-07T11:19:58Z

@icyJoseph

The problem I have with this is that, I notice that ever time I switch back and forth from the landing page to a page with a plot, it reloads the plot (for a second or two).

The current behavior:

At the beginning (when you visit the site), it takes a while to load, but once it is loaded, whenever you switch to any page, there is no wait time.

This new behavior:

Switching between any page with a plot is extremely fast, but if I accidentally visit the landing page again, I have to reload the plot again. I think it shouldn't have to reload the plot again...

What do you think?

mithi · 2020-07-07T12:24:19Z

src/App.js

@@ -115,34 +116,26 @@ class App extends React.Component {
     * * * * * * * * * * * * * */

    render() {
+        const { plot, ...rest } = this.state
+        const routeProps = {


Could there be a better way to structure this than pushing about 15 props on route?

Maybe using contexts ?

Yes, I actually a branch where I've built the Provider pattern, so that'll come up next.

I just needed to understand correctly which pieces of state are running through the App. For instance in that branch I have it divided as hexapod params, plot data, and the rest of settings. And that results in 3 context providers.

Edit
Forgot to say here that the main reason I did this strange props arrangement was to keep the render method under 25 lines and pass the code checks.

icyJoseph · 2020-07-07T13:25:44Z

@icyJoseph

The problem I have with this is that, I notice that ever time I switch back and forth from the landing page to a page with a plot, it reloads the plot (for a second or two).

The current behavior:

At the beginning (when you visit the site), it takes a while to load, but once it is loaded, whenever you switch to any page, there is no wait time.

This new behavior:

Switching between any page with a plot is extremely fast, but if I accidentally visit the landing page again, I have to reload the plot again. I think it shouldn't have to reload the plot again...

What do you think?

@mithi

With all the changes now done, I started to use the HTML attribute hidden, which is the pretty much the same as using display: none, with has full browser support.

Now the site will load fast, and in the background it'll be downloading and rendering the HexapodPlot page, if the user clicks too fast on a route, they might catch the Loading state. Once loaded, you can go to the Root, back and forth and there won't be reloading of the Plot.

See how it works now :D

mithi · 2020-07-07T14:19:21Z

src/components/Nav.js


-const Nav = () => <NavBullets />
+const Nav = React.memo(() => <NavBullets />)


Thanks for using React.memo in Nav 😄

mithi · 2020-07-07T14:23:31Z

See how it works now :D

Great! Do you think the plot should also be hidden in the 404 page?

I'm thinking about using profiler to measure the performance gains of your changes 😃
https://reactjs.org/docs/optimizing-performance.html#profiling-components-with-the-chrome-performance-tab

icyJoseph · 2020-07-07T15:07:40Z

There should be some gains. Here's an article I wrote somw time ago about using these tricks :)

I am actually not sure about what the 404 page should show. Up to you ^^

icyJoseph · 2020-07-07T15:39:24Z

Found and fixed a bug. Needed to update updateIkParams and updatePatternPose to updateIk and updatePose.

mithi · 2020-07-07T16:55:56Z

@icyJoseph

There should be some gains. Here's an article I wrote some time ago about using these tricks :)

Thanks for sharing! I'll read it! (And also your other articles)

Instead of displaying a 404 page, shouldn't we just redirect to the landing page?

Also, just a heads-up I'm implementing a walking gait page
( https://github.com/mithi/hexapod/tree/add-walking )

Also, I'm moving AlertBox and PoseTable inside the InverseKinematicsPage (as it should have had been from the very start! ) That means I'm removing some things in the App's state, simplifying the App component. 😃

icyJoseph · 2020-07-08T07:19:30Z

@mithi

Instead of displaying a 404 page, shouldn't we just redirect to the landing page?

Alright, I think it should be fine in this case.

Also, just a heads-up I'm implementing a walking gait page
( https://github.com/mithi/hexapod/tree/add-walking )

Oh that sound amazing 😮

Also, I'm moving AlertBox and PoseTable inside the InverseKinematicsPage (as it should have had been from the very start! ) That means I'm removing some things in the App's state, simplifying the App component.

Sweet! less global state is always welcomed

icyJoseph · 2020-07-08T07:59:32Z

I have not added the 404 Redirect back to room and merged master into this branch. Please do check that the Walking Gait is working as expected 😄

Gonna try to fix the issues throughout the day.

…de quality

icyJoseph · 2020-07-08T09:18:26Z

Had to use the hooks -> render props -> HoCs pattern architecture to remove about 7 code climate issues, now there's only two regarding params injected to some of the routes. I might fix it today.

The walking animations looks AMAZING! 🎉

mithi · 2020-07-08T09:47:29Z

I might fix it today.

Please do take your time and do not rush or hurry. Thanks for all your time, effort and all your hard work!
I will also study how to use the profiler to measure the performance gains of your changes. It will take me a bit of time.

mithi · 2020-07-08T10:32:58Z

src/App.js

+                        <Routes {...routeProps} onPageLoad={this.onPageLoad} />
+                    </Route>
+                    <Route>
+                        <Redirect to="/" />


icyJoseph · 2020-07-08T12:47:27Z

Ah :D I think this PR is ready for some serious merging considerations 💯

icyJoseph added 11 commits July 6, 2020 15:54

feat: break into smaller bundles based on pages

78fa2fc

feat: lazy load Hexapod and use semantic routing to display components

7b95d18

chore: Add jsdom-sixteen and bump up @testing-library/react

bb32e8d

test: Use waitFor to deal with Suspenseful modules

971eb20

test: Update test, the plot no longer renders hidden on landing page

fd2f65b

feat: Start to load Plotly before first DOM commit

66fd770

chore: Configure jest collectCoverageFrom

d178edb

feat: Implement a sleep util w/ test

fd9e0e9

test: Pass testId to mocked Plotly div

f53cc02

feat: Lazy load Plotly with retry option

9af7154

Merge branch 'master' into feat/reduce-bundle-size

c10bfd4

icyJoseph added 2 commits July 7, 2020 11:11

refactor: Shrink App's render method

2f5ccd4

test: fix coverage collect globbing pattern

f0c087e

mithi reviewed Jul 7, 2020

View reviewed changes

src/components/pages/NoMatch.js Outdated

export default () => (

<div className="no-match">

<h1>404</h1>

Copy link

Owner

mithi Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really cool 404 page 😊

mithi reviewed Jul 7, 2020

View reviewed changes

feat: render HexapodPlot ASAP, but hide it if not needed

429a4a9

icyJoseph force-pushed the feat/reduce-bundle-size branch from 58921fe to 429a4a9 Compare July 7, 2020 13:23

mithi reviewed Jul 7, 2020

View reviewed changes

icyJoseph added 2 commits July 7, 2020 17:36

Merge branch 'master' into feat/reduce-bundle-size

195cb69

fix: Update ikParams and PatternPose function names

ffc108a

icyJoseph added 4 commits July 8, 2020 09:36

feat: Redirect back to Root on 404

da2d6f5

feat: Less aggressive preloading

5ad2a60

Merge branch 'master' into feat/reduce-bundle-size

057a30d

refactor: Remove duplicated function names

1f40226

refactor: Various fixes, and code quality improvements

c077bf2

icyJoseph force-pushed the feat/reduce-bundle-size branch from cbdde42 to c077bf2 Compare July 8, 2020 08:50

refactor: Build a hooks -> render props -> HoC pattern to increase co…

f9b9bb6

…de quality

icyJoseph added 2 commits July 8, 2020 11:40

refactor: Hoist handlers Provider to App

7d9f154

refactor: LegPattern Page does not need params props anymore

3dea501

mithi reviewed Jul 8, 2020

View reviewed changes

src/App.js Outdated

<Routes {...routeProps} onPageLoad={this.onPageLoad} />

</Route>

<Route>

<Redirect to="/" />

Copy link

Owner

mithi Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 👍

icyJoseph added 5 commits July 8, 2020 12:43

refactor: Breakout HexapodParams into a Provider pattern

a4c652a

chore: Merge master including global state shrinking

84d7856

fix: Update PoseTable source of props and AlertBox message body

30a110c

refactor: Turn App into a functional component

edeb6e3

refactor: Move away custom hooks

a51f61f

mithi linked an issue Jul 8, 2020 that may be closed by this pull request

Optimize: Reduce Bundle Size Further #55

Open

4 tasks

mithi mentioned this pull request Jul 22, 2020

Performance Issues #130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/reduce bundle size #106

Feat/reduce bundle size #106

icyJoseph commented Jul 7, 2020 •

edited

Loading

codecov bot commented Jul 7, 2020 •

edited

Loading

icyJoseph commented Jul 7, 2020

mithi commented Jul 7, 2020

mithi Jul 7, 2020

mithi Jul 7, 2020

mithi commented Jul 7, 2020 •

edited

Loading

mithi Jul 7, 2020

icyJoseph Jul 7, 2020 •

edited

Loading

icyJoseph commented Jul 7, 2020 •

edited

Loading

The current behavior:

This new behavior:

What do you think?

mithi Jul 7, 2020

mithi commented Jul 7, 2020

icyJoseph commented Jul 7, 2020

icyJoseph commented Jul 7, 2020 •

edited

Loading

mithi commented Jul 7, 2020 •

edited

Loading

icyJoseph commented Jul 8, 2020

icyJoseph commented Jul 8, 2020

icyJoseph commented Jul 8, 2020

mithi commented Jul 8, 2020

mithi Jul 8, 2020

icyJoseph commented Jul 8, 2020


		const Nav = () => <NavBullets />
		const Nav = React.memo(() => <NavBullets />)

Feat/reduce bundle size #106

Are you sure you want to change the base?

Feat/reduce bundle size #106

Conversation

icyJoseph commented Jul 7, 2020 • edited Loading

codecov bot commented Jul 7, 2020 • edited Loading

Codecov Report

icyJoseph commented Jul 7, 2020

mithi commented Jul 7, 2020

For comparison

Before

After

mithi Jul 7, 2020

Choose a reason for hiding this comment

mithi Jul 7, 2020

Choose a reason for hiding this comment

mithi commented Jul 7, 2020 • edited Loading

The current behavior:

This new behavior:

What do you think?

mithi Jul 7, 2020

Choose a reason for hiding this comment

icyJoseph Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

icyJoseph commented Jul 7, 2020 • edited Loading

The current behavior:

This new behavior:

What do you think?

mithi Jul 7, 2020

Choose a reason for hiding this comment

mithi commented Jul 7, 2020

icyJoseph commented Jul 7, 2020

icyJoseph commented Jul 7, 2020 • edited Loading

mithi commented Jul 7, 2020 • edited Loading

icyJoseph commented Jul 8, 2020

icyJoseph commented Jul 8, 2020

icyJoseph commented Jul 8, 2020

mithi commented Jul 8, 2020

mithi Jul 8, 2020

Choose a reason for hiding this comment

💯 👍

icyJoseph commented Jul 8, 2020

icyJoseph commented Jul 7, 2020 •

edited

Loading

codecov bot commented Jul 7, 2020 •

edited

Loading

mithi commented Jul 7, 2020 •

edited

Loading

icyJoseph Jul 7, 2020 •

edited

Loading

icyJoseph commented Jul 7, 2020 •

edited

Loading

icyJoseph commented Jul 7, 2020 •

edited

Loading

mithi commented Jul 7, 2020 •

edited

Loading