De-duplicate Webpack build, especially `components/` #929

nylen · 2017-05-29T19:01:44Z

While looking at #926 I noticed that we are probably duplicating some code in the components/ directory across built files. For example, we import IconButton in both editor/header/tools/preview-button.js and blocks/editable/format-toolbar.js.

If we split components/ out into a separate build + entry point, we should be able to resolve this issue, and also make our common components available via wp.components, which seems good for reusability.

This PR is an attempt at doing this, but it's not working for some reason. Here are the build sizes before this PR:

$ ls -la */build/*.js
271232 blocks/build/index.js
  3119 date/build/index.js
466975 editor/build/index.js
 14447 element/build/index.js
 17481 i18n/build/index.js

and after:

$ ls -la */build/*.js
271230 blocks/build/index.js
118012 components/build/index.js
  3119 date/build/index.js
467031 editor/build/index.js
 14389 element/build/index.js
 17481 i18n/build/index.js

`component` depends on `element`. :(

BE-Webdesign · 2017-05-29T21:15:05Z

Looks good to me. Will we also need to go through the files and change where modules are being imported from? Or is webpack smart enough to not duplicate once a build is created for that directory? Here is the current list to illustrate what I mean.

gutenberg\blocks\block-controls\index.js
9:import Toolbar from 'components/toolbar';

gutenberg\blocks\editable\format-toolbar.js
4:import IconButton from 'components/icon-button';
5:import Toolbar from 'components/toolbar';

gutenberg\blocks\editable\index.js
13:import Toolbar from 'components/toolbar';

gutenberg\blocks\library\button\index.js
4:import IconButton from 'components/icon-button';

gutenberg\blocks\library\embed\index.js
4:import Button from 'components/button';
5:import Placeholder from 'components/placeholder';

gutenberg\blocks\library\image\index.js
4:import Placeholder from 'components/placeholder';

gutenberg\blocks\media-upload-button\index.js
6:import Button from 'components/button';

gutenberg\components\panel\body.js
5:import IconButton from 'components/icon-button';

gutenberg\components\placeholder\index.js
10:import Dashicon from 'components/dashicon';

gutenberg\editor\block-mover\index.js
9:import IconButton from 'components/icon-button';

gutenberg\editor\block-switcher\index.js
11:import IconButton from 'components/icon-button';
12:import Dashicon from 'components/dashicon';

gutenberg\editor\header\mode-switcher\index.js
9:import Dashicon from 'components/dashicon';

gutenberg\editor\header\saved-state\index.js
10:import Dashicon from 'components/dashicon';

gutenberg\editor\header\tools\index.js
10:import IconButton from 'components/icon-button';

gutenberg\editor\header\tools\preview-button.js
9:import IconButton from 'components/icon-button';

gutenberg\editor\header\tools\publish-button.js
9:import Button from 'components/button';

gutenberg\editor\inserter\index.js
10:import IconButton from 'components/icon-button';

gutenberg\editor\inserter\menu.js
11:import Dashicon from 'components/dashicon';
12:import withFocusReturn from 'components/higher-order/with-focus-return';

gutenberg\editor\modes\visual-editor\block.js
14:import Toolbar from 'components/toolbar';

gutenberg\editor\sidebar\block-inspector\index.js
9:import Panel from 'components/panel';
10:import PanelHeader from 'components/panel/header';
11:import PanelBody from 'components/panel/body';

gutenberg\editor\sidebar\discussion-panel\index.js
11:import PanelBody from 'components/panel/body';
12:import FormToggle from 'components/form-toggle';

gutenberg\editor\sidebar\featured-image\index.js
11:import Button from 'components/button';
12:import PanelBody from 'components/panel/body';
14:import Spinner from 'components/spinner';

gutenberg\editor\sidebar\index.js
9:import withFocusReturn from 'components/higher-order/with-focus-return';

gutenberg\editor\sidebar\post-excerpt\index.js
10:import PanelBody from 'components/panel/body';

gutenberg\editor\sidebar\post-schedule\clock.js
5:import Button from 'components/button';

gutenberg\editor\sidebar\post-settings\index.js
10:import Panel from 'components/panel';
11:import PanelHeader from 'components/panel/header';
12:import IconButton from 'components/icon-button';

gutenberg\editor\sidebar\post-status\index.js
11:import PanelBody from 'components/panel/body';
12:import FormToggle from 'components/form-toggle';

gutenberg\editor\sidebar\post-trash\index.js
10:import Button from 'components/button';
11:import Dashicon from 'components/dashicon';

nylen · 2017-05-30T01:50:56Z

Or is webpack smart enough to not duplicate once a build is created for that directory?

This is the part that's not working yet - I would expect to see the 118kb file for components/build/index.js, and then I would expect to see slightly more than this 118kb (due to existing duplication) moved out of blocks/build/index.js and editor/build/index.js.

I suspect something is still missing in the webpack config.

nylen · 2017-05-30T03:24:17Z

Before this PR there's actually a lot of duplication in the built files. Multiple copies of Dashicon, for example. The latest commits improve things somewhat - here are the new build sizes:

$ ls -la */build/*.js
253181 blocks/build/index.js
 96260 components/build/index.js
  3119 date/build/index.js
334667 editor/build/index.js
 14554 element/build/index.js
 17483 i18n/build/index.js

However, there is still a lot of duplication - IconButton and Dashicon are each present in the blocks, editor, and components builds. To achieve full separation between our built files, we will have to make further changes:

- import IconButton from 'components/icon-button';
+ import { IconButton } from 'components';

- Fix entry point order to match actual dependencies - Fix externals (`wp.*` externals not needed)

nylen · 2017-05-30T04:22:36Z

This PR should be ready for review now. It shaves 200 KB off of the production build size by adding a new components/build/index.js output file and then eliminating a lot of duplication across the various files built by Webpack.

It works by registering each of our top-level entry points like components as an external library using the Webpack externals option. Behind the scenes, then, Webpack will transform import { thing } from 'components' into something that translates to wp.components.thing. Previously, all imports from e.g. components were being included in each output file.

I saw #901 (comment) after I already started restructuring imports in this way. The one difference is that everything should be imported from components directly, instead of subdirectories like components/higher-order.

Current sizes:

$ ls -la */build/*.js
179431 blocks/build/index.js
 96343 components/build/index.js
  3119 date/build/index.js
258059 editor/build/index.js
 14550 element/build/index.js
 17483 i18n/build/index.js

Final statistics:

773254  Build size on `master`
568985  Build size on this branch
204269  Savings (26% of original size)

youknowriad · 2017-05-30T08:50:29Z

components/index.js

+export { default as Spinner } from './spinner';
+export { default as Toolbar } from './toolbar';
+
+export { default as withFocusReturn } from './higher-order/with-focus-return';


I don't think we should export this in the root module. I'm thinking we should create a index.js in the ./higher-order/ folder to be able to import those as import { withFocusReturn } from 'components/higher-order';

Oh I just saw your comment #929 (comment)

I still think we should make a distinction between importing components and HoC.

How do we make this work like wp.components.something behind the scenes?

I think this could be solved separately, in any case.

BE-Webdesign

Nice Work! Verified that the build size is significantly smaller. LGTM, any issues can be cycled back to in another PR I think as this resolves things for the current structure.

nylen · 2017-05-30T14:44:53Z

I've created #941 to discuss next steps.

ellatrix · 2017-05-31T10:08:28Z

I think this broke master. There's an error if you open the inspector.

youknowriad · 2017-05-31T10:45:49Z

Confirmed this broke a lot of components. I fixed this in a branch 17741f4 but we should probably fix this separately

Build components/ directory separately

228442a

nylen added [Type] Build Tooling Issues or PRs related to build tooling [Status] In Progress Tracking issues with work in progress labels May 29, 2017

nylen requested review from youknowriad, aduth and BE-Webdesign May 29, 2017 19:01

Fix the build

d151f3e

`component` depends on `element`. :(

nylen mentioned this pull request May 29, 2017

Adds button component tests. #926

Merged

nylen added 2 commits May 29, 2017 21:55

Enqueue wp.components script

ebb69e5

Consolidate entry points into one variable

463a2be

nylen force-pushed the add/components-build branch from 4d4416b to 9b29ebc Compare May 30, 2017 03:21

Register all wp. libraries as Webpack externals

63b2db4

nylen force-pushed the add/components-build branch from 9b29ebc to 63b2db4 Compare May 30, 2017 03:39

nylen added 4 commits May 29, 2017 23:40

Restructure imports to top-level entry points only

e768d79

Fix duplicate imports

a47cf55

Fix naming of withFocusReturn HoC

3ba82c4

Fix the tests

8cac43e

- Fix entry point order to match actual dependencies - Fix externals (`wp.*` externals not needed)

nylen changed the title ~~Build components/ directory separately~~ De-duplicate components/ and other parts of Webpack build May 30, 2017

nylen changed the title ~~De-duplicate components/ and other parts of Webpack build~~ De-duplicate Webpack build, especially components/ May 30, 2017

nylen removed the [Status] In Progress Tracking issues with work in progress label May 30, 2017

Fix wp-components stylesheet and dependencies on editor page

81a4bf4

youknowriad reviewed May 30, 2017

View reviewed changes

BE-Webdesign approved these changes May 30, 2017

View reviewed changes

nylen mentioned this pull request May 30, 2017

Module and import strategy #941

Closed

nylen merged commit ccf8728 into master May 30, 2017

nylen deleted the add/components-build branch May 30, 2017 14:45

This was referenced May 30, 2017

Add keycodes file #940

Merged

Dashicon not loading properly in test environment when loaded from externals. #944

Closed

youknowriad mentioned this pull request May 31, 2017

Build: Fix components exports after refactor #957

Merged

nylen mentioned this pull request May 31, 2017

Add Perl script to list JS dependencies and suggest a load order #965

Closed

aduth mentioned this pull request Aug 4, 2017

Linting: Restrict WordPress import path access #2211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

De-duplicate Webpack build, especially `components/` #929

De-duplicate Webpack build, especially `components/` #929

nylen commented May 29, 2017

BE-Webdesign commented May 29, 2017

nylen commented May 30, 2017

nylen commented May 30, 2017

nylen commented May 30, 2017

youknowriad May 30, 2017

youknowriad May 30, 2017

nylen May 30, 2017

BE-Webdesign left a comment

nylen commented May 30, 2017

ellatrix commented May 31, 2017

youknowriad commented May 31, 2017

De-duplicate Webpack build, especially components/ #929

De-duplicate Webpack build, especially components/ #929

Conversation

nylen commented May 29, 2017

BE-Webdesign commented May 29, 2017

nylen commented May 30, 2017

nylen commented May 30, 2017

nylen commented May 30, 2017

youknowriad May 30, 2017

Choose a reason for hiding this comment

youknowriad May 30, 2017

Choose a reason for hiding this comment

nylen May 30, 2017

Choose a reason for hiding this comment

BE-Webdesign left a comment

Choose a reason for hiding this comment

nylen commented May 30, 2017

ellatrix commented May 31, 2017

youknowriad commented May 31, 2017

De-duplicate Webpack build, especially `components/` #929

De-duplicate Webpack build, especially `components/` #929