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

Investigate bundle size regression #157

Closed
bvaughn opened this issue Mar 4, 2019 · 5 comments
Closed

Investigate bundle size regression #157

bvaughn opened this issue Mar 4, 2019 · 5 comments

Comments

@bvaughn
Copy link
Owner

bvaughn commented Mar 4, 2019

Adding FixedSizeList from v1.5.2 to a create-react-app app adds +2.07 KB to the production build. Doing the same with a build from master same increases the size by an additional +2.43 KB. My guess is that this is caused by 97bd7e0, given that almost nothing else has changed since 1.5.2 was released.

@bvaughn
Copy link
Owner Author

bvaughn commented Mar 4, 2019

Looks like this was the result of how I was testing (yarn build and yarn add /path/to/local/project). When I npm pack first then the build size is only slightly larger than v1.5.2 (+2.23 KB for the #150 branch vs 2.07 KB for v1.5.2)

This is still an increase (~160 bytes), but there's some added behavior as well so some small amount of increase is expected.

@bvaughn
Copy link
Owner Author

bvaughn commented Mar 4, 2019

I tried adding terser-js by way of rollup-plugin-terser, specifically hoping to take advantage of its mangle properties option to shorten "private" methods like _onScrollHorizontal but this seemed to break tree shaking which naturally resulted in a significant increase in bundle size. This might be worth revisiting at some point in the future but I'm out of time now. For the time being, I backed it out (0f24e9a).

For the moment, I've updated Rollup to the latest release and tweaked the domHelpers util a bit (7130e03). I think the size increase is reasonable.

Testing with create-react-app shows v1.5.2 adding 2.09 KB and master currently adds 2.26 KB (a delta of 0.16 but with a few new features added). I'm happy enough for now.

@bvaughn bvaughn closed this as completed Mar 4, 2019
@bvaughn
Copy link
Owner Author

bvaughn commented Mar 4, 2019

Out of curiosity, I tried a stupid naive approach of minifying "private" variables myself using rollup-plugin-postprocess and was able to shave an additional 336 B of the bundle size when using the list component.

diff --git a/rollup.config.js b/rollup.config.js
index c05d722..af49852 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -1,6 +1,7 @@
 import babel from 'rollup-plugin-babel';
 import commonjs from 'rollup-plugin-commonjs';
 import nodeResolve from 'rollup-plugin-node-resolve';
+import postprocess from 'rollup-plugin-postprocess';
 
 import pkg from './package.json';
 
@@ -8,6 +9,33 @@ const input = './src/index.js';
 
 const external = id => !id.startsWith('.') && !id.startsWith('/');
 
+const chars = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ$_'.split('');
+const replacementMap = new Map();
+let replacementIndex = 0;
+
+// Inspired by terser-js
+function base54(num) {
+    let string = '';
+    let base = 54;
+    num++;
+    do {
+        num--;
+        string += chars[num % base];
+        num = Math.floor(num / base);
+        base = 64;
+    } while (num > 0);
+    return string;
+}
+
+const postprocessConfig = [
+  [/([ \.])(_[a-zA-Z]+)/, (string, prefix, id) => {
+    if (!replacementMap.has(id)) {
+      replacementMap.set(id, base54(replacementIndex++));
+    }
+    return prefix + replacementMap.get(id);
+  }]
+];
+
 export default [
   {
     input,
@@ -23,6 +51,7 @@ export default [
       }),
       nodeResolve(),
       commonjs(),
+      postprocess(postprocessConfig),
     ],
   },
 
@@ -40,6 +69,7 @@ export default [
       }),
       nodeResolve(),
       commonjs(),
+      postprocess(postprocessConfig),
     ],
   },
 ];

This is a pretty sketchy approach though ^ 😄 My regex is definitely not expressive enough (e.g. it would miss variables wrapped in "()"). I was just curious how much potential savings is there.

@bvaughn
Copy link
Owner Author

bvaughn commented Mar 4, 2019

I think the tree shaking problem might have actually been due to the /*#__PURE__*/ comments being removed. Looks like this has nothing to do with my mangle regex. Also looks like setting comments: 'all' and compress: { side_effects: false } doesn't stop it from happening.

@bvaughn
Copy link
Owner Author

bvaughn commented Mar 4, 2019

The above issue looks potentially related to TrySound/rollup-plugin-terser#15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant