Skip to content

Add side effects property to components package.json#18911

Merged
senadir merged 1 commit intomasterfrom
add/side-effects-to-components
Dec 5, 2019
Merged

Add side effects property to components package.json#18911
senadir merged 1 commit intomasterfrom
add/side-effects-to-components

Conversation

@senadir
Copy link
Copy Markdown
Contributor

@senadir senadir commented Dec 4, 2019

Description

This PR adds sideEffects: false to @wordpress/components in order to enable tree-shaking for that library, having tree shaking reduces build sizes by a significant amount.

before sideEffects

File sizes after gzip:

  256.37 KB  build/static/js/2.7889de9e.chunk.js
  15.45 KB   build/static/css/2.8225445d.chunk.css
  780 B      build/static/js/runtime-main.c4aaabd0.js
  317 B      build/static/js/main.e8a8781c.chunk.js

after sideEffects

File sizes after gzip:

  40.61 KB (-215.77 KB)  build/static/js/2.0646c13c.chunk.js
  15.45 KB               build/static/css/2.8225445d.chunk.css
  780 B                  build/static/js/runtime-main.c4aaabd0.js
  305 B (-12 B)          build/static/js/main.fa03990e.chunk.js

@senadir senadir added [Type] Performance Related to performance efforts npm Packages Related to npm packages [Package] Components /packages/components labels Dec 4, 2019
@senadir senadir mentioned this pull request Dec 4, 2019
Merged
talldan
talldan approved these changes Dec 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Looks good. I wasn't able to spot any side effects in the package, so this makes sense 👍

@senadir senadir merged commit 00589e7 into master Dec 5, 2019
@senadir senadir deleted the add/side-effects-to-components branch December 5, 2019 06:04
@gziolo gziolo requested a review from jsnajdr December 5, 2019 11:16
@gziolo
Copy link
Copy Markdown
Member

gziolo commented Dec 5, 2019

gutenberg/packages/components/package.json

Lines 54 to 57 in 9eea50b

"sideEffects": [
"src/date-time/index.js",
"src/keyboard-shortcuts/index.js"
]

@talldan, in the past you marked two files, do you remember why?

@gziolo
Copy link
Copy Markdown
Member

gziolo commented Dec 5, 2019

gutenberg/packages/components/src/date-time/index.js

Line 6 in a8c0ca9

import 'react-dates/initialize';

gutenberg/packages/components/src/keyboard-shortcuts/index.js

Lines 4 to 5 in a8c0ca9

import Mousetrap from 'mousetrap';
import 'mousetrap/plugins/global-bind/mousetrap-global-bind';

I think this is what is concerning in this package. I'm not saying we should change anything, but we should definitely seek a way to avoid those magic imports to make reasoning easier.

@jsnajdr
Copy link
Copy Markdown
Member

jsnajdr commented Dec 5, 2019 •
edited
Loading

I think setting sideEffects to a simple false is fine and special treatment for date-time and keyboard-shortcuts is not necessary.

Yes, these two packages import something that has side-effects, but these don't need to happen unless the consumer of @wordpress/components explicitly imports KeyboardShortcuts or DatePicker. If the consumer imports and used them, then mousetrap and react-dates will initialize and do their side-effects. If the consumer doesn't use these components, the libraries are not imported, because no one needs them.

It would be a problem only if some other component relied implicitly on mousetrap or react-dates initialization, relying on other components to do the import for them.

@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
@Aljullu Aljullu mentioned this pull request Dec 12, 2019
Merged
3 tasks
@sirreal sirreal mentioned this pull request Jan 9, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@talldan talldan talldan approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@youknowriad youknowriad Awaiting requested review from youknowriad

@jsnajdr jsnajdr Awaiting requested review from jsnajdr

Assignees

No one assigned

Labels

npm Packages Related to npm packages [Package] Components /packages/components [Type] Performance Related to performance efforts

Projects

None yet

Milestone

Gutenberg 7.1

Development

Successfully merging this pull request may close these issues.

5 participants

@senadir @gziolo @jsnajdr @talldan @youknowriad