Webpack migration #842

Hyperkid123 · 2020-06-18T13:14:45Z

Replace parcel by webpack and optimize the js built output

TO DO

js build
js build beta
watch js
optimize chunks

Major development change

I was constantly dealing with cache issues in development. That was solved by adding a chunk hash to the root chrome.js file. However, this means that in development we also need to serve the HTML generated from pug templates.

I do believe we want to include the hash if the app is using code splitting to avoid any potential issue with state root JS asset. But this change makes the development bit harder. You have to also run some app and you need to manually add the root element to body. However, without that the development was a serious pain because I was constantly dealing with outdated JS assets and the app was sporadically failing when using minimized bundles. @ryelo @karelhala your thoughts?

Hyperkid123 · 2020-06-19T12:23:37Z

The issue with broken production build was fixed.

Now there is a another one when the navigation and the user are sometimes not set in the redux store. (they are not set fast enough)

Hyperkid123 · 2020-06-23T10:45:11Z

The issue with missing identity information are resolved. Now I will start to clean up the mess I did in the JS files and move to other build tasks.

codecov-commenter · 2020-06-23T13:58:05Z

Codecov Report

Merging #842 into master will decrease coverage by 1.48%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##           master     #842      +/-   ##
==========================================
- Coverage   58.18%   56.69%   -1.49%     
==========================================
  Files          48       49       +1     
  Lines         959      933      -26     
  Branches      187      189       +2     
==========================================
- Hits          558      529      -29     
- Misses        324      325       +1     
- Partials       77       79       +2

Impacted Files	Coverage Δ
src/js/App/Header/ToolbarToggle.js	`52.94% <ø> (ø)`
src/js/App/Sidenav/Navigation.js	`87.23% <ø> (ø)`
src/js/auth.js	`28.57% <ø> (-8.93%)`	⬇️
src/js/externalDependencies.js	`0.00% <0.00%> (ø)`
src/js/inventory/details.js	`0.00% <0.00%> (ø)`
src/js/inventory/index.js	`10.52% <0.00%> (-0.59%)`	⬇️
src/js/jwt/insights/entitlements.js	`10.00% <ø> (-25.72%)`	⬇️
src/js/jwt/logger.js	`75.00% <ø> (-5.00%)`	⬇️
src/js/nav/sourceOfTruth.js	`20.00% <ø> (ø)`
src/js/rbac/rbac.js	`20.00% <ø> (-80.00%)`	⬇️
... and 15 more

Hyperkid123 · 2020-06-24T06:53:46Z

The migration is complete.

@karelhala @ryelo Can I ask you to do check this build? You know the platform better than me and it is very likely that I have forgot to check some app or a feature.

Looking at the build output, we are doing fairly OK. The main issue is inventory dependencies. It pulls around 1.4mb and the fact that chrome itself has some strangely large dependencies. I am optimistic that we can fix it but we will most likely need to start with FCE.

ryelo · 2020-06-24T13:47:04Z

@Hyperkid123 Yes! I'll dig through some of this later today

ryelo · 2020-06-29T15:33:17Z

@Hyperkid123 I tested this with both the landing page and RBAC and I'm getting a bunch of chunk failures. The apps are still rendering, but they leave the nav and masthead unusable.

Hyperkid123 · 2020-06-30T06:40:02Z

@ryelo interesting I am not seeing that. Maybe some cache issue? I will try and reproduce it.

Hyperkid123 · 2020-06-30T10:20:39Z

@ryelo there a bug with the public path. There was missing / and the chunks were loaded from apps/chrome/jsChunkName instead of apps/chrome/js/ChunkName. I wonder why it was not an issue when I was setting that up 🤔.

I was experiencing some strange cache issues while switching to the webpack. Some times it was trying to load assets that no longer existed. I hope I wont be an issue for you.

ryelo · 2020-06-30T15:09:56Z

@Hyperkid123 So this one seems better for some pages, but a couple more bugs I'm getting:

404s for skeleton.css and spinner.css
Uncaught TypeError: Cannot read property 'call' of undefined

Although I also can't get the sourcemaps to load, so it's kind of difficult to see where number 2 is coming from.

Hyperkid123 · 2020-06-30T15:19:08Z

Uncaught TypeError: Cannot read property 'call' of undefined

@ryelo I don't see that when I run it locally 🤔 in which app are you seeing that?

The skeleton is coming from FCE. It tries to reach totally wrong resource at

/apps/chrome/node_modules/@redhat-cloud-services/frontend-components/components/skeleton.css

EDIT: The undefined call appears in RBAC. I will find what is going on there.

Hyperkid123 · 2020-06-30T15:24:58Z

@ryelo well, the CSS invalid routes is actually not caused by this PR. I am setting that currently on ci-beta env.

Hyperkid123 · 2020-07-01T08:30:23Z

So the undefined call is caused by using the Suspense/lazy combination. Turning off side effects for production optimizations (is set to true by default) might help with that.

EDIT: Side effects did not help
We are probably dealing with conflicting webpack chunks between chrome and apps. I wonder if there some setting that could eliminate the problem. See the app chunks are overriding the chrome chunks which causes the runtime crash (back is overriding the unauthorized header for example). The obvious solution is to stop with the code splitting of chrome. That would only increase the initial chunk size of the landing page and the logout page but still I would like to find some solution to it.

Hyperkid123 · 2020-07-01T09:44:58Z

@ryelo as usual the simplest solution is the best. I've renamed the webpackJSONP function for the chrome. That fixed the problem. For reference: https://webpack.js.org/configuration/output/#outputjsonpfunction

The issue was that sometimes (completely random) the application has overwritten the chrome chunks inside the JSONP object. These objects are publically accessible on window so if you have multiple instances of webpack on one page, webpack will try to put everything into the same object.

This works fine until you start using lazy-loaded chunks.

Chrome now has its own namespace called wpJsonpChromeInstance. Apps can still use the default webpackJsonp.

ryelo · 2020-07-01T14:16:44Z

@Hyperkid123 That's a wild journey! I'll have some time tomorrow that I can go over it again. Thanks for looking over this stuff!

karelhala · 2020-07-09T08:16:35Z

Just dropping this here from our conversation @Hyperkid123 https://github.com/jherr/wp5-intro-video-code so we don't forget about it. Great point using this for inventory @rvsia!

ryelo · 2020-07-28T17:27:49Z

@Hyperkid123 I'm getting some errors on build

ERROR in ./node_modules/@redhat-cloud-services/frontend-components-inventory/index.js
Module not found: Error: Can't resolve 'PFReactTable' in '/Users/rlong/Insights/insights-chrome/node_modules/@redhat-cloud-services/frontend-components-inventory'
 @ ./node_modules/@redhat-cloud-services/frontend-components-inventory/index.js 2:127-150
 @ ./src/js/inventory.js
 @ ./src/js/entry.js
 @ ./src/js/chrome.js

ERROR in ./node_modules/@redhat-cloud-services/frontend-components-inventory/index.js
Module not found: Error: Can't resolve 'customReact' in '/Users/rlong/Insights/insights-chrome/node_modules/@redhat-cloud-services/frontend-components-inventory'
 @ ./node_modules/@redhat-cloud-services/frontend-components-inventory/index.js 2:104-126
 @ ./src/js/inventory.js
 @ ./src/js/entry.js
 @ ./src/js/chrome.js

ERROR in ./node_modules/@redhat-cloud-services/frontend-components-inventory/index.js
Module not found: Error: Can't resolve 'reactRedux' in '/Users/rlong/Insights/insights-chrome/node_modules/@redhat-cloud-services/frontend-components-inventory'
 @ ./node_modules/@redhat-cloud-services/frontend-components-inventory/index.js 2:375-396
 @ ./src/js/inventory.js
 @ ./src/js/entry.js
 @ ./src/js/chrome.js

Hyperkid123 · 2020-07-29T06:39:24Z

Ah. I guess this was caused by the latest inventory refactoring eh? We will probably need to adjust the externals configuration.

Hyperkid123 · 2020-07-29T07:01:23Z

@ryelo ok I've added new inventory dependencies package names to externals and the build is running (also tried to run it locally and worked with the catalog app).

karelhala

All of the externals from config should be reverted to previous module.exports since that's slightly different from what export default does. If we want to use imports and exports (without module.exports) we have to figure out how to spread the hot loaded deps.

import React, { Component } from 'react' won't work if used in export default mode, but will work with module.exports since we are exporting default and named exports as well.

Also, looks like running npm run build won't use newer bundles, might be reasonable to purge build/js and build/css when running build or start script.

karelhala · 2020-07-29T09:08:36Z

config/webpack.config.js

+        chunkFilename: '[name].[chunkhash].js',
+        jsonpFunction: 'wpJsonpChromeInstance'
+    },
+    externals: {


Externals won't work as these are not externals but rhat pulled from different app. I think that resolve.alias is more suitable for this

resolve: { alias: { PFReactTable: path.resolve(__dirname, './patternfly-table-externals.js'), '@patternfly/react-table': path.resolve(__dirname, './patternfly-table-externals.js'), customReact: path.resolve(__dirname, './react-external.js'), reactRedux: path.resolve(__dirname, './react-redux-external.js'), 'react-router-dom': path.resolve(__dirname, './react-router-dom-externals.js'), graphql: path.resolve(__dirname, './graphql-externals.js') } }

karelhala · 2020-07-29T09:13:41Z

src/js/inventory.js

@@ -4,6 +4,6 @@ export default (dependencies) => {
    setDependencies(dependencies);

    return import('./inventoryStyles').then(
-        () => import('@redhat-cloud-services/frontend-components-inventory')
+        () => import(/* webpackChunkName: "inventory-styles" */ '@redhat-cloud-services/frontend-components-inventory')


I'd name this inventory instead of inventory-styles. Also might be resonable to name styles as well

return import(/* webpackChunkName: "inventory" */ './inventoryStyles').then( () => import(/* webpackChunkName: "inventory" */ '@redhat-cloud-services/frontend-components-inventory') )

karelhala · 2020-07-29T09:14:30Z

src/js/inventoryStyles.js

@@ -1,2 +1,7 @@
+/**


Should be fixed.

Hyperkid123 · 2020-07-29T09:31:38Z

All of the externals from config should be reverted to previous module.exports since that's slightly different from what export default does. If we want to use imports and exports (without module.exports) we have to figure out how to spread the hot loaded deps.

@karelhala yeah we would have to use require('foo').default. Do you know what consumes these packages? I would like to test it out.

Hyperkid123 · 2020-08-11T14:48:30Z

@karelhala so I have fixed the npm scripts. A couple of notes

the watch could never work before because of the pug. It requires css/js files to be built before so I pre-build these two and only then run the parallel
also, there is no watch option on the pug build script so I have removed it
In order to pass additional options to the build:js and it's beta alternative I had to bump it out of the npm-run-all scope (it was't propagating the webpack options to the subcommand)

So the start and watch scripts should be working now. I can't think of anything else we need to do to finish this up.

It will remove any potential issues with stale root chunk. That helps enormously when making any changes and using local chrome. The downside is, that you need to serve the HTML locally as well.

Webpack has the same defalt jsonP value for every bundle. If you run multiple instances of webpack on one page these instances are merged and chunks can be replacd which will cause issue when using lazy loading. By renaming chrome webpack instance, we eliminate potential conflicts and we can keep using lazy loading.

Hyperkid123 force-pushed the webpack-migration branch from 16e78c6 to d0110fd Compare June 19, 2020 12:22

Hyperkid123 force-pushed the webpack-migration branch from d0110fd to 5ab30e7 Compare June 23, 2020 10:31

Hyperkid123 force-pushed the webpack-migration branch 2 times, most recently from b6d222d to 9e53e52 Compare June 23, 2020 13:53

Hyperkid123 force-pushed the webpack-migration branch from be874fc to 4042e73 Compare June 30, 2020 11:25

Hyperkid123 changed the title ~~[WIP] Webpack migration~~ Webpack migration Jun 30, 2020

Hyperkid123 force-pushed the webpack-migration branch from f3872b8 to f59931c Compare June 30, 2020 15:28

Hyperkid123 force-pushed the webpack-migration branch from c4fb1f6 to cdcf3a7 Compare July 7, 2020 07:06

Hyperkid123 force-pushed the webpack-migration branch from 9c64192 to 9bd8f02 Compare July 20, 2020 13:57

Hyperkid123 force-pushed the webpack-migration branch from 01c596b to c5dc4f9 Compare July 28, 2020 11:16

karelhala reviewed Jul 29, 2020

View reviewed changes

Hyperkid123 force-pushed the webpack-migration branch from d441c58 to 781371f Compare August 7, 2020 07:01

karelhala mentioned this pull request Aug 7, 2020

Pass pf-react-core to inventory RedHatInsights/insights-remediations-frontend#191

Merged

Hyperkid123 force-pushed the webpack-migration branch 2 times, most recently from c0d9451 to 7ebb35b Compare August 11, 2020 11:48

Hyperkid123 force-pushed the webpack-migration branch from fc133ff to f4a9e50 Compare August 17, 2020 08:02

Hyperkid123 added 16 commits August 27, 2020 09:39

Migrate JS build to webpack

5c928e5

Fixed tests after JS build migration

46899b8

Use webpack for js beta build

0013da3

use webpack for js watch

267d288

Added analyze:js command

7eabb1b

Fixed webpack public path

328d6a5

Added chunk hash to root chrome JS asset

2a6c0e2

It will remove any potential issues with stale root chunk. That helps enormously when making any changes and using local chrome. The downside is, that you need to serve the HTML locally as well.

Fixed snapshots after PF update

71a81b8

Fix log import in analytics.

7cafe3c

Update webpack inventory externals config.

d2c4bc4

Use modules for inventory externals.

b8f7719

Remove obsolete webpack aliases.

163fb9b

Update inventory component.

1de878b

Add chunk names to lazy loaded components.

5a23cff

Fix build scripts after webpack migration

179c766

Hyperkid123 force-pushed the webpack-migration branch from f4a9e50 to 179c766 Compare August 27, 2020 07:40

karelhala approved these changes Aug 27, 2020

View reviewed changes

karelhala merged commit c4a70ad into RedHatInsights:master Aug 27, 2020

Hyperkid123 deleted the webpack-migration branch August 27, 2020 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webpack migration #842

Webpack migration #842

Hyperkid123 commented Jun 18, 2020 •

edited

Loading

Hyperkid123 commented Jun 19, 2020

Hyperkid123 commented Jun 23, 2020

codecov-commenter commented Jun 23, 2020 •

edited

Loading

Hyperkid123 commented Jun 24, 2020

ryelo commented Jun 24, 2020

ryelo commented Jun 29, 2020

Hyperkid123 commented Jun 30, 2020

Hyperkid123 commented Jun 30, 2020 •

edited

Loading

ryelo commented Jun 30, 2020

Hyperkid123 commented Jun 30, 2020 •

edited

Loading

Hyperkid123 commented Jun 30, 2020

Hyperkid123 commented Jul 1, 2020 •

edited

Loading

Hyperkid123 commented Jul 1, 2020 •

edited

Loading

ryelo commented Jul 1, 2020

karelhala commented Jul 9, 2020 •

edited

Loading

ryelo commented Jul 28, 2020

Hyperkid123 commented Jul 29, 2020

Hyperkid123 commented Jul 29, 2020

karelhala left a comment

karelhala Jul 29, 2020

karelhala Jul 29, 2020

karelhala Jul 29, 2020

Hyperkid123 commented Jul 29, 2020

Hyperkid123 commented Aug 11, 2020 •

edited

Loading

Webpack migration #842

Webpack migration #842

Conversation

Hyperkid123 commented Jun 18, 2020 • edited Loading

TO DO

Major development change

Hyperkid123 commented Jun 19, 2020

Hyperkid123 commented Jun 23, 2020

codecov-commenter commented Jun 23, 2020 • edited Loading

Codecov Report

Hyperkid123 commented Jun 24, 2020

ryelo commented Jun 24, 2020

ryelo commented Jun 29, 2020

Hyperkid123 commented Jun 30, 2020

Hyperkid123 commented Jun 30, 2020 • edited Loading

ryelo commented Jun 30, 2020

Hyperkid123 commented Jun 30, 2020 • edited Loading

Hyperkid123 commented Jun 30, 2020

Hyperkid123 commented Jul 1, 2020 • edited Loading

Hyperkid123 commented Jul 1, 2020 • edited Loading

ryelo commented Jul 1, 2020

karelhala commented Jul 9, 2020 • edited Loading

ryelo commented Jul 28, 2020

Hyperkid123 commented Jul 29, 2020

Hyperkid123 commented Jul 29, 2020

karelhala left a comment

Choose a reason for hiding this comment

karelhala Jul 29, 2020

Choose a reason for hiding this comment

karelhala Jul 29, 2020

Choose a reason for hiding this comment

karelhala Jul 29, 2020

Choose a reason for hiding this comment

Hyperkid123 commented Jul 29, 2020

Hyperkid123 commented Aug 11, 2020 • edited Loading

Hyperkid123 commented Jun 18, 2020 •

edited

Loading

codecov-commenter commented Jun 23, 2020 •

edited

Loading

Hyperkid123 commented Jun 30, 2020 •

edited

Loading

Hyperkid123 commented Jun 30, 2020 •

edited

Loading

Hyperkid123 commented Jul 1, 2020 •

edited

Loading

Hyperkid123 commented Jul 1, 2020 •

edited

Loading

karelhala commented Jul 9, 2020 •

edited

Loading

Hyperkid123 commented Aug 11, 2020 •

edited

Loading