Fix treeshaking #2232

redallen · 2019-07-31T14:56:40Z

https://jira.coreos.com/browse/CONSOLE-1651

Let's go down a rabbit hole. Currently, our vendor bundle has a > 10MB stat size and looks like this:

It should NOT include all of patternfly-react since we don't use all of patternfly-react. Likewise, it should not include the huge c3 dependency of patternfly-react's charts, which are unused in any files under the frontend folder. It also shouldn't include all of @patternfly/react-core and numerous other sections of packages.

This critical line of the webpack config is causing NONE of our code to be treeshaken: exclude: /node_modules/,. This is because of yarn workspaces -- it can most obviously be seen right in the entrypoint of the webpack config: entry: [...'@console/app',]. Well whoops, @console/app is under node_modules, if only by a symlink to packages/console-app so it will never be tree shaken.

This is easily solvable with a regex like /node_modules\/(?!@console\/)/, but new problems arise:

We still have loader={() => import('patternfly-react').then(m => m.Switch)} of which Webpack only sees the import and then (ironically) loads the whole module synchronously for the bundle. This can be solved.
The Kubevirt plugin imports kubevirt-web-ui-components, which only has a js export and can't be tree shaken. If esm modules are provided, the bundle size decreases by half to 5.5MB:

I'm currently venturing upstream to kubevirt-web-ui-components to provide some ESM exports. @suomiy @mareklibra .

openshift-ci-robot · 2019-07-31T14:56:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: redallen
To complete the pull request process, please assign rhamilto
You can assign the PR to them by writing /assign @rhamilto in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2019-07-31T14:56:54Z

Hi @redallen. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

alecmerdler · 2019-07-31T15:16:45Z

/ok-to-test

christianvogt · 2019-07-31T15:29:17Z

This doesn't seem right to me to have ts-loader process node_modules. Is tree shaking completely broken right now or only some particular modules?

redallen · 2019-07-31T17:42:02Z

Yeah, this one is actually looking more complicated than I thought a few hours ago.

Currently, this PR results in bundles like this:

where the vendors~main chunk seems to be correctly shaken down from 10MB to 5.33MB. However, there's now additional chunks that just contain those modules that should be tree shaken, so I thought this was a fix too soon.

This issue can most prevalently be seen by c3 and d3 which are a part of patternfly-react charts, but aren't used anywhere. All dependencies seem to be wholly included, whether used or not.

In a test webpack project, patternfly-react is tree shaken just fine using JS. I'll slowly build my minimum test case scenario to have a configuration like yours.

christianvogt · 2019-08-01T14:12:43Z

@redallen is there an issue tracking this with specific details as to what modules are affected and captures the difference between the current and the expected modules to include to some extent?

d3 is being used by dev-console. Perhaps we can look into better ways to tree shake it. But first an answer to the above would be great.

redallen · 2019-08-02T18:17:32Z

I just opened #2251 to track this. c3 is the largest offender that can only included by patternfly-react (it depends on it for charts):

➜  frontend git:(upmaster) yarn why c3 
yarn why v1.15.2
[1/4] Why do we have the module "c3"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "c3@0.4.23"
info Reasons this module exists
   - "_project_#patternfly" depends on it
   - Hoisted from "_project_#patternfly#c3"
   - Hoisted from "_project_#patternfly-react#react-c3js#c3"
   - Hoisted from "_project_#patternfly-react#patternfly#c3"
   - Hoisted from "_project_#patternfly-react-extensions#patternfly#c3"

redallen · 2019-08-16T17:04:28Z

Giving this another shot since kubevirt/web-ui-components#536 and kubevirt/web-ui-components#539 were merged. Bundle size isn't as good as I hoped for since kubevirt-plugin/src/plugin is larger than anticipated (4MB), but it still improves the vendor bundle by 3.2 MB down to 7.53MB and shaves nearly 300kb off the gzipped bundle:

spadgett · 2019-08-16T20:15:26Z

With code splitting, I wouldn't expect any kubevirt dependency to appear in the main vendor bundle. We should try to track down what's pulling it in.

openshift-ci-robot · 2019-08-16T20:16:10Z

@redallen: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-console-olm	`ab7335b`	link	`/test e2e-aws-console-olm`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

spadgett · 2019-08-16T20:16:40Z

frontend/public/components/operator-lifecycle-manager/create-operand.tsx

@@ -331,7 +332,7 @@ export const CreateOperandForm: React.FC<CreateOperandFormProps> = (props) => {
    }
    if (field.capabilities.includes(SpecCapability.booleanSwitch)) {
      return <AsyncComponent
-        loader={() => import('patternfly-react').then(m => m.Switch)}
+        loader={() => Switch}


No reason to use AsyncComponent now. Are we certain this change is needed?

We might just switch to the PF4 switch.

It's needed or we need to make a Webpack chunk for it so all of patternfly-react isn't included.

It's needed or we need to make a Webpack chunk for it so all of patternfly-react isn't included.

Right, I see it now 👍

spadgett · 2019-08-16T20:16:57Z

frontend/public/components/operator-lifecycle-manager/descriptors/spec/index.tsx

@@ -56,7 +57,7 @@ const BooleanSwitch: React.FC<SpecCapabilityProps> = (props) => {

  return <div className="co-spec-descriptor--switch">
    <AsyncComponent
-      loader={() => import('patternfly-react').then(m => m.Switch)}
+      loader={() => Switch}


No reason to use AsyncComponent any longer if we're already importing Switch

spadgett · 2019-08-16T20:18:12Z

frontend/webpack.config.ts

@@ -39,7 +36,7 @@ const config: webpack.Configuration = {
      { test: /\.glsl$/, loader: 'raw!glslify' },
      {
        test: /(\.jsx?)|(\.tsx?)$/,
-        exclude: /node_modules/,
+        exclude: /node_modules\/(?!(@console)\/)/,


Are we sure this is right?

I'm sure this is the magic line. The current exclude excludes everything since all @console/* references actually are in node_modules/@console/*.

I'm sure this is the magic line. The current exclude excludes everything since all @console/* references actually are in node_modules/@console/*.

I'm still not sure. If I only bump kubevirt-web-ui-components with no other changes, I get a main vendor bundle size of 7.52MB, which is the same as #2232 (comment)

spadgett · 2019-08-16T20:20:29Z

/cc @vojtechszocs

spadgett · 2019-08-16T20:44:43Z

OK I see what's pulling kubevirt-web-ui-components into the main vendor bundle.

kubevirt-plugin/src/index.tsx ->
kubevirt-plugin/src/components/dashboards-page/overview-dashboard/inventory ->
kubevirt-web-ui-components

Commenting that out with no other changes results in this:

openshift-ci-robot · 2019-08-17T05:37:47Z

@redallen: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

spadgett · 2019-08-17T15:09:21Z

Thanks for your work on this!

I think the right approach is to make these changes separately and check how each impacts bundle sizes. This PR has several different changes, and it's hard to evaluate what impact each has in a single PR.

I hope you don't mind, but I opened #2383 specifically for the Switch change. I removed AsyncComponent in those files entirely as it's not needed.

I suspect the two big wins will be:

Enabling dead code elimination for kubevirt-web-ui-components (already done now, thanks @redallen!)
Removing kubevirt-web-ui-components from the main vendor bundle

I looked at (2). It's getting pulled in for the inventory card on the dashboard. Unfortunately, it needs kubevirt-web-ui-components for getVmStatus. This is not easy to just copy into the kubevirt plugin. It relies on a bunch of helpers spread across several modules. I think the kubevirt team will need to investigate.

In general, we should look at code splitting static plugins. Ideally, inactive plugins should not increase the vendor bundle size.

I also want to add a check in CI that prevents us from accidentally blowing up the main vendor bundle. It's far too easy to do, and it's difficult to track back and find the change that caused the problem initially.

rawagner · 2019-08-19T11:27:54Z

Thanks for your work on this!

I think the right approach is to make these changes separately and check how each impacts bundle sizes. This PR has several different changes, and it's hard to evaluate what impact each has in a single PR.

I hope you don't mind, but I opened #2383 specifically for the Switch change. I removed AsyncComponent in those files entirely as it's not needed.

I suspect the two big wins will be:

Enabling dead code elimination for kubevirt-web-ui-components (already done now, thanks @redallen!)

Removing kubevirt-web-ui-components from the main vendor bundle

I looked at (2). It's getting pulled in for the inventory card on the dashboard. Unfortunately, it needs kubevirt-web-ui-components for getVmStatus. This is not easy to just copy into the kubevirt plugin. It relies on a bunch of helpers spread across several modules. I think the kubevirt team will need to investigate.

I'll create a PR to move getVmStatus to kubevirt-plugin

In general, we should look at code splitting static plugins. Ideally, inactive plugins should not increase the vendor bundle size.

I also want to add a check in CI that prevents us from accidentally blowing up the main vendor bundle. It's far too easy to do, and it's difficult to track back and find the change that caused the problem initially.

redallen · 2019-08-19T13:24:42Z

Closing in favor of #2383 #2372, and a follow-up PR to move getVmStatus to kubevirt-plugin.

vojtechszocs · 2019-08-19T17:02:48Z

In general, we should look at code splitting static plugins. Ideally, inactive plugins should not increase the vendor bundle size.

@spadgett It depends on what you mean by "active" plugin.

The current definition of "active" plugin is:

detected via packages/console-app/package.json or provided via CONSOLE_PLUGINS env. variable (override the standard detection mechanism)
included in the webpack-generated bundle
loaded as part of Console startup (public/plugins.ts) => part of initial main-chunk

Arguably, the above definition feels closer to "bundled" or "loaded" plugin, so maybe we should rename that.

Anyway, a Console plugin contributes N extensions, each of which may be gated by feature flags (based on #2269). It's perfectly possible to have one extension not gated by anything and another one gated by something. However, flags used to gate (potentially) every extension can only be resolved after you load the plugin's entry module.

@spadgett Can you please elaborate on the "inactive" plugin concern?

spadgett · 2019-08-19T17:24:43Z

By active, I mean a plugin that has at least one extension point whose required flag is set.

However, flags used to gate (potentially) every extension can only be resolved after you load the plugin's entry module.

That's currently true. Can there be a static declaration that says only load the plugin if at least one of these flags is set? Ideally no plugin code runs until we know it's needed.

Otherwise, we need a rule that a plugin entry module cannot pull in any 3rd party dependencies, otherwise they're included in the main vendor bundle even when not needed. I'm not sure how to enforce this, though.

vojtechszocs · 2019-08-19T18:22:48Z

By active, I mean a plugin that has at least one extension point whose required flag is set.

This can be solved by loading plugins at build time, which is already possible, e.g. in plugin-stats.js

loadActivePlugins(resolvePluginPackages())

Some extensions are always-on (*), the rest is gate-able by flags via required & disallowed arrays.

(*) ModelFeatureFlag, ModelDefinition

At Console startup, we can load the extensions which are either 1, always-on or 2, whose flags constraint is empty (no required or disallowed flags specified). The remaining extensions can be put to use as soon as their flags constraint is satisfied.

However, flags used to gate (potentially) every extension can only be resolved after you load the plugin's entry module.

That's currently true. Can there be a static declaration that says only load the plugin if at least one of these flags is set? Ideally no plugin code runs until we know it's needed.

We could extend Console plugin definition in package.json like so:

"consolePlugin": {
  "entry": "src/plugin.ts",
  "entryFlags": ["FOO", "BAR"]
}

so that the plugin as a whole would be loaded only when FOO and BAR flags are not pending anymore, i.e. flagValue !== undefined.

However, this way, we exclude extensions which are either 1, always-on or 2, whose flags constraint is empty.

A much better approach IMO is, at build time, to read/process extensions into logical categories, treating "Console plugin" only as a developer / build-time representation of extensions.

Otherwise, we need a rule that a plugin entry module cannot pull in any 3rd party dependencies, otherwise they're included in the main vendor bundle even when not needed. I'm not sure how to enforce this, though.

I'd add that rule regardless of the above. It makes perfect sense. Plugin entry module should only contribute (export) extension objects. This module on its own shouldn't have any external dependencies or trigger any side-effects.

I've talked about this with @christianvogt some time ago, having the plugin entry module as JSON file would be ideal from extension definition perspective (but less practical from code perspective).

To enforce this, we can use ESLint rules that forbid static or dynamic imports.

christianvogt · 2019-08-20T19:35:19Z

There's a reason plugin frameworks typically use a declarative, non-code, approach.
We need to be very strict that a plugin should never import code that has real size to it. Any code references should be using dynamic imports.

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 31, 2019

openshift-ci-robot requested review from jhadvig and rhamilto July 31, 2019 14:57

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 31, 2019

alecmerdler self-requested a review July 31, 2019 15:17

redallen changed the title ~~have ts-loader parse node_modules for webpack to treeshake~~ Don't use node module resolution for packages we want to treeshake Aug 6, 2019

redallen mentioned this pull request Aug 6, 2019

add babel esm and umd exports kubevirt/web-ui-components#536

Merged

spadgett added this to the v4.2 milestone Aug 9, 2019

redallen mentioned this pull request Aug 15, 2019

Add module to package.json kubevirt/web-ui-components#539

Merged

redallen added 4 commits August 16, 2019 11:54

Update async switch import

78584f6

Update webpack and kubevirt dependencies

8cf5913

Use ts-loader for @Console packages

7baea37

Manually resolve plugins

e6ea06b

redallen force-pushed the fix/tree-shaking branch from adcdd1c to e6ea06b Compare August 16, 2019 15:56

Remove getActivePluginsModule test

6b59eef

redallen force-pushed the fix/tree-shaking branch from 070c34b to 6b59eef Compare August 16, 2019 16:04

cleanup and fix linting

da74f8d

openshift-ci-robot added the component/dev-console Related to dev-console label Aug 16, 2019

redallen added 2 commits August 16, 2019 14:16

fix jest tests

f167417

remove cyclical imports

ab7335b

redallen changed the title ~~Don't use node module resolution for packages we want to treeshake~~ Fix treeshaking Aug 16, 2019

redallen mentioned this pull request Aug 16, 2019

Webpack isn't treeshaking library bundles #2251

Closed

spadgett reviewed Aug 16, 2019

View reviewed changes

openshift-ci-robot requested a review from vojtechszocs August 16, 2019 20:20

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2019

redallen closed this Aug 19, 2019

rawagner mentioned this pull request Aug 19, 2019

move getVmStatus to kubevirt plugin #2394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix treeshaking #2232

Fix treeshaking #2232

redallen commented Jul 31, 2019 •

edited by spadgett

Loading

openshift-ci-robot commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019

alecmerdler commented Jul 31, 2019

christianvogt commented Jul 31, 2019

redallen commented Jul 31, 2019

christianvogt commented Aug 1, 2019

redallen commented Aug 2, 2019

redallen commented Aug 16, 2019 •

edited

Loading

spadgett commented Aug 16, 2019

openshift-ci-robot commented Aug 16, 2019 •

edited

Loading

spadgett Aug 16, 2019

redallen Aug 16, 2019

spadgett Aug 16, 2019

spadgett Aug 16, 2019

spadgett Aug 16, 2019

redallen Aug 16, 2019

spadgett Aug 16, 2019

spadgett commented Aug 16, 2019

spadgett commented Aug 16, 2019

openshift-ci-robot commented Aug 17, 2019

spadgett commented Aug 17, 2019

rawagner commented Aug 19, 2019

redallen commented Aug 19, 2019

vojtechszocs commented Aug 19, 2019 •

edited

Loading

spadgett commented Aug 19, 2019

vojtechszocs commented Aug 19, 2019

christianvogt commented Aug 20, 2019

Fix treeshaking #2232

Fix treeshaking #2232

Conversation

redallen commented Jul 31, 2019 • edited by spadgett Loading

openshift-ci-robot commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019

alecmerdler commented Jul 31, 2019

christianvogt commented Jul 31, 2019

redallen commented Jul 31, 2019

christianvogt commented Aug 1, 2019

redallen commented Aug 2, 2019

redallen commented Aug 16, 2019 • edited Loading

spadgett commented Aug 16, 2019

openshift-ci-robot commented Aug 16, 2019 • edited Loading

spadgett Aug 16, 2019

Choose a reason for hiding this comment

redallen Aug 16, 2019

Choose a reason for hiding this comment

spadgett Aug 16, 2019

Choose a reason for hiding this comment

spadgett Aug 16, 2019

Choose a reason for hiding this comment

spadgett Aug 16, 2019

Choose a reason for hiding this comment

redallen Aug 16, 2019

Choose a reason for hiding this comment

spadgett Aug 16, 2019

Choose a reason for hiding this comment

spadgett commented Aug 16, 2019

spadgett commented Aug 16, 2019

openshift-ci-robot commented Aug 17, 2019

spadgett commented Aug 17, 2019

rawagner commented Aug 19, 2019

redallen commented Aug 19, 2019

vojtechszocs commented Aug 19, 2019 • edited Loading

spadgett commented Aug 19, 2019

vojtechszocs commented Aug 19, 2019

christianvogt commented Aug 20, 2019

redallen commented Jul 31, 2019 •

edited by spadgett

Loading

redallen commented Aug 16, 2019 •

edited

Loading

openshift-ci-robot commented Aug 16, 2019 •

edited

Loading

vojtechszocs commented Aug 19, 2019 •

edited

Loading