New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framework: Decrease build size by fixing import statements #3531

Merged
merged 1 commit into from Nov 17, 2017

Conversation

Projects
None yet
4 participants
@gziolo
Member

gziolo commented Nov 17, 2017

Description

This PR updates import statements that were using legacy format. It turned out this caused blocks chunk to be also bundled together with editor chunk doubling the size of the chunk.

editor chunk has decreased size from 1.02 MB to 551 kB for production build. This is before compression.

It's worth mention that side-effects from blocks chunk were executed twice including all block registrations!

Before

                         Asset     Size               Chunks                    Chunk Names
           i18n/build/index.js  17.5 kB                    5  [emitted]         i18n
         blocks/build/index.js   552 kB                 1, 4  [emitted]  [big]  blocks
     components/build/index.js   262 kB                 2, 4  [emitted]  [big]  components
          utils/build/index.js  23.4 kB                    3  [emitted]         utils
        element/build/index.js  9.51 kB                    4  [emitted]         element
         editor/build/index.js  1.02 MB        0, 1, 3, 4, 5  [emitted]  [big]  editor
           date/build/index.js  3.08 kB                    6  [emitted]         date
      ./blocks/build/style.css  42.7 kB  0, 1, 3, 4, 5, 1, 4  [emitted]         editor, blocks
./blocks/build/edit-blocks.css  31.3 kB  0, 1, 3, 4, 5, 1, 4  [emitted]         editor, blocks
  ./components/build/style.css  30.8 kB                 2, 4  [emitted]         components
      ./editor/build/style.css  82.3 kB        0, 1, 3, 4, 5  [emitted]         editor

After

                         Asset     Size  Chunks                    Chunk Names
           i18n/build/index.js  17.6 kB       5  [emitted]         i18n
         blocks/build/index.js   551 kB       0  [emitted]  [big]  blocks
     components/build/index.js   260 kB       2  [emitted]  [big]  components
          utils/build/index.js  23.4 kB       3  [emitted]         utils
        element/build/index.js  9.52 kB       4  [emitted]         element
         editor/build/index.js   551 kB       1  [emitted]  [big]  editor
           date/build/index.js  3.08 kB       6  [emitted]         date
      ./blocks/build/style.css  42.7 kB       0  [emitted]         blocks
./blocks/build/edit-blocks.css  31.3 kB       0  [emitted]         blocks
  ./components/build/style.css  30.8 kB       2  [emitted]         components
      ./editor/build/style.css  82.3 kB       1  [emitted]         editor

How Has This Been Tested?

Manually.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 17, 2017

Codecov Report

Merging #3531 into master will increase coverage by 0.15%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3531      +/-   ##
=========================================
+ Coverage   34.85%     35%   +0.15%     
=========================================
  Files         261     261              
  Lines        6717    6733      +16     
  Branches     1225    1229       +4     
=========================================
+ Hits         2341    2357      +16     
  Misses       3691    3691              
  Partials      685     685
Impacted Files Coverage Δ
components/form-file-upload/index.js 71.42% <ø> (ø) ⬆️
components/higher-order/with-api-data/provider.js 25% <ø> (ø) ⬆️
...ents/post-taxonomies/hierarchical-term-selector.js 1.35% <ø> (ø) ⬆️
editor/components/post-visibility/utils.js 100% <ø> (ø) ⬆️
editor/components/word-count/index.js 0% <ø> (ø) ⬆️
editor/store-defaults.js 100% <ø> (ø) ⬆️
blocks/library/image/image-size.js 0% <ø> (ø) ⬆️
components/higher-order/with-api-data/index.js 83.95% <ø> (ø) ⬆️
editor/writing-flow/index.js 0% <0%> (ø) ⬆️
editor/store-persist.js 100% <0%> (ø) ⬆️
... and 1 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 5c93d78...a46bff1. Read the comment docs.

codecov bot commented Nov 17, 2017

Codecov Report

Merging #3531 into master will increase coverage by 0.15%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3531      +/-   ##
=========================================
+ Coverage   34.85%     35%   +0.15%     
=========================================
  Files         261     261              
  Lines        6717    6733      +16     
  Branches     1225    1229       +4     
=========================================
+ Hits         2341    2357      +16     
  Misses       3691    3691              
  Partials      685     685
Impacted Files Coverage Δ
components/form-file-upload/index.js 71.42% <ø> (ø) ⬆️
components/higher-order/with-api-data/provider.js 25% <ø> (ø) ⬆️
...ents/post-taxonomies/hierarchical-term-selector.js 1.35% <ø> (ø) ⬆️
editor/components/post-visibility/utils.js 100% <ø> (ø) ⬆️
editor/components/word-count/index.js 0% <ø> (ø) ⬆️
editor/store-defaults.js 100% <ø> (ø) ⬆️
blocks/library/image/image-size.js 0% <ø> (ø) ⬆️
components/higher-order/with-api-data/index.js 83.95% <ø> (ø) ⬆️
editor/writing-flow/index.js 0% <0%> (ø) ⬆️
editor/store-persist.js 100% <0%> (ø) ⬆️
... and 1 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 5c93d78...a46bff1. Read the comment docs.

@gziolo gziolo requested review from aduth, mcsf and jorgefilipecosta Nov 17, 2017

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 17, 2017

Member

Actually, those changes are straightforward and block my work. I will merge it without review.

Member

gziolo commented Nov 17, 2017

Actually, those changes are straightforward and block my work. I will merge it without review.

@gziolo gziolo merged commit 7a1e75e into master Nov 17, 2017

3 checks passed

codecov/project 35% (+0.15%) compared to 5c93d78
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gziolo gziolo deleted the update/decrease-build-size branch Nov 17, 2017

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Nov 17, 2017

Contributor

Good one. Should we lint against this? Looks like one of those things that's easy to slip by, even more so when refactoring.

Contributor

mcsf commented Nov 17, 2017

Good one. Should we lint against this? Looks like one of those things that's easy to slip by, even more so when refactoring.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 17, 2017

Member

Awesome finding 👍 It is good that you merged it, so you could advance I also did a set of smoke tests, and some grepping to find other cases and no problem found / other cases appeared.

Member

jorgefilipecosta commented Nov 17, 2017

Awesome finding 👍 It is good that you merged it, so you could advance I also did a set of smoke tests, and some grepping to find other cases and no problem found / other cases appeared.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 17, 2017

Member

I'm sure that it can be handled on the Webpack level. However, it would require some explorations how to avoid scanning sibling folders. I have a feeling that I saw once an article how to achieve it. Eslint might be more challenging to maintain if we ever add a new folder. I guess it wouldn't be hard to update but the trick is how to keep it in sync.

Member

gziolo commented Nov 17, 2017

I'm sure that it can be handled on the Webpack level. However, it would require some explorations how to avoid scanning sibling folders. I have a feeling that I saw once an article how to achieve it. Eslint might be more challenging to maintain if we ever add a new folder. I guess it wouldn't be hard to update but the trick is how to keep it in sync.

@@ -7,7 +7,7 @@ import { connect } from 'react-redux';
* WordPress dependencies
*/
import { getBlocks } from '../../selectors';

This comment has been minimized.

@youknowriad

youknowriad Nov 20, 2017

Contributor

This should probably move to "Internal dependencies"

@youknowriad

youknowriad Nov 20, 2017

Contributor

This should probably move to "Internal dependencies"

@@ -1,4 +1,4 @@
import { viewPort } from '../utils';

This comment has been minimized.

@youknowriad

youknowriad Nov 20, 2017

Contributor

Missing WordPress dependencies comment :)

@youknowriad

youknowriad Nov 20, 2017

Contributor

Missing WordPress dependencies comment :)

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 20, 2017

Member

@youknowriad did you open PR? I can fix it in a bit since I started it :)

Member

gziolo commented Nov 20, 2017

@youknowriad did you open PR? I can fix it in a bit since I started it :)

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 20, 2017

Contributor

No I did not :)

Contributor

youknowriad commented Nov 20, 2017

No I did not :)

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 20, 2017

Member

Opened #3561 to address that.

Member

gziolo commented Nov 20, 2017

Opened #3561 to address that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment