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.

Sign up for GitHub

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

Jump to bottom

chore(build): remove PropTypes from builds #3697

Merged
merged 1 commit into from
Apr 17, 2019
Merged

chore(build): remove PropTypes from builds #3697

merged 1 commit into from
Apr 17, 2019

Conversation

francoischalifour
Copy link
Member

@francoischalifour francoischalifour commented Apr 16, 2019 •
edited
Loading

Description

PropTypes were still mostly present in the bundles (both UMD and ES/CJS) despite the usage of the plugin babel-plugin-transform-react-remove-prop-types. As opposed to React InstantSearch, there's no reasons to expose PropTypes in our bundles. This PR aims at removing those.

  • For the UMD bundle, I introduced the ignore Rollup plugin to ignore the prop-types package.
  • For the ES/CJS bundle, I added { removeImport: true }, which defaults to false, to remove all traces of prop-types imports (see documentation).

The PropTypes remaining come from preact-rheostat, which will be addressed in a later PR.

Result

Before the change

┌────────────────────────────────────────────────────┐
│                                                    │
│   Destination: dist/instantsearch.development.js   │
│   Bundle Size:  1.03 MB                            │
│   Gzipped Size:  79.8 KB                           │
│                                                    │
└────────────────────────────────────────────────────┘

┌───────────────────────────────────────────────────────┐
│                                                       │
│   Destination: dist/instantsearch.production.min.js   │
│   Bundle Size:  284.36 KB                             │
│   Gzipped Size:  78.96 KB                             │
│                                                       │
└───────────────────────────────────────────────────────┘

After the change

┌────────────────────────────────────────────────────┐
│                                                    │
│   Destination: dist/instantsearch.development.js   │
│   Bundle Size:  1.02 MB                            │
│   Gzipped Size:  78.52 KB                          │
│                                                    │
└────────────────────────────────────────────────────┘

┌───────────────────────────────────────────────────────┐
│                                                       │
│   Destination: dist/instantsearch.production.min.js   │
│   Bundle Size:  280.44 KB                             │
│   Gzipped Size:  77.67 KB                             │
│                                                       │
└───────────────────────────────────────────────────────┘

Related

@francoischalifour francoischalifour requested a review from a team April 16, 2019 15:42
@algobot
Copy link
Contributor

algobot commented Apr 16, 2019 •
edited
Loading

Deploy preview for instantsearchjs ready!

Built with commit 158c432

https://deploy-preview-3697--instantsearchjs.netlify.com

@francoischalifour
Copy link
Member Author

Although the UMD is a bit smaller, the GZIP version seems to be a be bigger according to Bundlesize.

@Haroenv
Copy link
Contributor

Haroenv commented Apr 16, 2019

Did you look at the bundles themselves? It might just be decreased repetition. When looking with @tkrugg earlier I saw some more unexpected dependencies in the bundle

@francoischalifour
Copy link
Member Author

Yes, the bundles look fine regarding PropTypes. From what I understand, the only remaining PropTypes are the ones part of Preact internals.

@samouss
Copy link
Contributor

samouss commented Apr 17, 2019 •
edited
Loading

It looks like the propTypes are part of the build because of Rheostat. The module is a noop but we have the import inside the output. We might want to also remove them from it to actually save some space.

@francoischalifour
Copy link
Member Author

@samouss You mean in preact-rheostat itself, right? That would be in a future PR.

@samouss
Copy link
Contributor

samouss commented Apr 17, 2019

@francoischalifour Yes. You want to merge even with the size increase?

@francoischalifour
Copy link
Member Author

I'm not exactly sure of the Bundlesize report but this is the actual result: after this PR, the bundle is slightly lighter.

Before the change

┌────────────────────────────────────────────────────┐
│                                                    │
│   Destination: dist/instantsearch.development.js   │
│   Bundle Size:  1.03 MB                            │
│   Gzipped Size:  79.8 KB                           │
│                                                    │
└────────────────────────────────────────────────────┘

┌───────────────────────────────────────────────────────┐
│                                                       │
│   Destination: dist/instantsearch.production.min.js   │
│   Bundle Size:  284.36 KB                             │
│   Gzipped Size:  78.96 KB                             │
│                                                       │
└───────────────────────────────────────────────────────┘

After the change

┌────────────────────────────────────────────────────┐
│                                                    │
│   Destination: dist/instantsearch.development.js   │
│   Bundle Size:  1.02 MB                            │
│   Gzipped Size:  78.52 KB                          │
│                                                    │
└────────────────────────────────────────────────────┘

┌───────────────────────────────────────────────────────┐
│                                                       │
│   Destination: dist/instantsearch.production.min.js   │
│   Bundle Size:  280.44 KB                             │
│   Gzipped Size:  77.67 KB                             │
│                                                       │
└───────────────────────────────────────────────────────┘

@francoischalifour
Copy link
Member Author

francoischalifour commented Apr 17, 2019 •
edited
Loading

@samouss I updated the branch and this time, we can notice the bundle size decrease on Bundlesize.

@francoischalifour francoischalifour merged commit 5ec4609 into develop Apr 17, 2019
@francoischalifour francoischalifour deleted the chore/remove-bundle-prop-types branch April 17, 2019 16:54
eunjae-lee pushed a commit that referenced this pull request May 14, 2019
Revert "chore(build): remove PropTypes from builds (#3697)"
1745089 
This reverts commit 5ec4609.
eunjae-lee pushed a commit that referenced this pull request May 14, 2019
revert: chore(build): remove PropTypes from builds (#3697) (#3776)
1e6be79 
This reverts commit 5ec4609.
eunjae-lee pushed a commit that referenced this pull request May 20, 2019
v3.5.1
f4f05bd 
## [3.5.1](v3.5.0...v3.5.1) (2019-05-20)

### Bug Fixes

* **types:** improve types for voiceSearch ([#3778](#3778)) ([ed2d61a](ed2d61a))
* **types:** update UiState type ([#3777](#3777)) ([36e3a3d](36e3a3d))
* **voiceSearch:** remove event listeners on dispose ([#3779](#3779)) ([0e988cc](0e988cc))

### Reverts

* chore(build): remove PropTypes from builds ([#3697](#3697)) ([#3776](#3776)) ([1e6be79](1e6be79))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Haroenv Haroenv Haroenv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@francoischalifour @algobot @Haroenv @samouss