Skip to content
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

Add side effects property to components package.json #18911

Merged
merged 1 commit into from
Dec 5, 2019

Conversation

senadir
Copy link
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

Copy link
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
Member

gziolo commented Dec 5, 2019

"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
Member

gziolo commented Dec 5, 2019

import 'react-dates/initialize';

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
Member

jsnajdr commented Dec 5, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Package] Components /packages/components [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants