Skip to content

Improve bundle size #3

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

Closed
wants to merge 2 commits into from
Closed

Conversation

tunnckoCore
Copy link

@tunnckoCore tunnckoCore commented Feb 9, 2017

can't deal with the unicorn/explicit-length-check sorry... tried eslint disabling, seems to not work. I'm not xo user.

In reality with better tooling (rollup + uglify) you get 488 bytes minified and 316 bytes gzip with zopfli compression - and that is umd bundle which adds 30-50 bytes wrapper.

Browserify + ESMangle isn't good. Using them it seems 2x more minified size. And that's because browserify's wrapper is around 300-400 bytes minified.

As about the created bundles from the "build" script.. they even not included in the npm package? files field includes only index and there's no main field?

@@ -3,61 +3,45 @@
/* Expose. */
module.exports = parse;

/* Characters */
var dot = '.'.charCodeAt(0);
var hash = '#'.charCodeAt(0);
Copy link
Author

Choose a reason for hiding this comment

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

is there any value? is it so important? it add bytes.

Copy link
Member

Choose a reason for hiding this comment

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

Using charCodeAt is faster than charCode or bracket notation.
And this is code that could be run a lot, so it could matter a little.

index.js Outdated
}

if (className.length !== 0) {
if (className.length) {
Copy link
Author

Choose a reason for hiding this comment

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

ooooh... tried to disable the wrong line, haha

index.js Outdated
@@ -41,7 +41,7 @@ function parse(selector) {
}
}

if (className.length) {
if (className.length) { // eslint-disable-line unicorn/explicit-length-check
Copy link
Author

@tunnckoCore tunnckoCore Feb 9, 2017

Choose a reason for hiding this comment

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

we can add >0, but... 2 more bytes for nothing serious. Really don't like such rules. It will be 0, so, falsey value, so won't pass the check, so not make sense.

@wooorm
Copy link
Member

wooorm commented Feb 9, 2017

@tunnckoCore Hi, interesting work! I’m going to decline however.

  1. I really like clear code: I prefer readable code, and your suggestions makes things less readable in my opinion
  2. I really like fast code. Some of your suggestions are slower (albeit slightly)

Here’s the difference:

-function d(n){var h=null,i=[],j=n||'',k='div',c,e=null,f=-1,g,m=j.length,d,l;c={type:'element',tagName:null,properties:{},children:[]},e=null;while(++f<=m)g=j.charCodeAt(f),(!g||g===a||g===b)&&(d=j.slice(l,f),d&&(e===a?i.push(d):e===b?h=d:k=d),l=f+1,e=g);return c.tagName=k,h&&(c.properties.id=h),i.length!==0&&(c.properties.className=i),c}c.exports=d;var a='.'.charCodeAt(0),b='#'.charCodeAt(0)
+function b(a){var b=0,f=[],g,h,c={type:'element',tagName:'div',properties:{},children:[]};a=a||'';while(b<=a.length){var d=a[b++];if(!d||d==='.'||d==='#'){var e=a.slice(h,b-1);e&&(g==='.'?f.push(e):g==='#'?c.properties.id=e:c.tagName=e),h=b,g=d}}return f.length&&(c.properties.className=f),c}a.exports=b

(the first line is the current code)

And here’s a comparison table of the two:

name compression .js min.js
current gzipped 877 B 650 B
suggested gzipped 823 B 599 B

Thats such a small difference! There’s probably much better ways to make your sight faster or smaller. It’s really not worth it discussing such a small change, in my opinion, if the down sides are performance and readability.

@wooorm wooorm closed this Feb 9, 2017
@wooorm
Copy link
Member

wooorm commented Feb 9, 2017

In reality with better tooling (rollup + uglify) you get 488 bytes minified and 316 bytes gzip with zopfli compression - and that is umd bundle which adds 30-50 bytes wrapper.

Yes, in reality. You should use whatever you like! Zopfli all the things.

Browserify + ESMangle isn't good. Using them it seems 2x more minified size. And that's because browserify's wrapper is around 300-400 bytes minified.

I really like browserify! Esmangle too. I think they’re good :) I’d rather remove a font or image from my static site, or increase caching on some files by a day or two: those would result in much more significant performance increases.

As about the created bundles from the "build" script.. they even not included in the npm package? files field includes only index and there's no main field?

They’re example distributions. I suggest you do that yourself. Including distributions on npm doesn’t make sense to me, you should bundle stuff yourself!

However, they’re nice to have for some users, and are included on the releases. A nice addition here is that you can see the difference in size across versions on that page.

@tunnckoCore
Copy link
Author

tunnckoCore commented Feb 9, 2017

Readability... It is absolutely clear what is what. More vars is just a noise.

Separate var name = 'div' and later tagName: null is absolutely no sense and waste of bytes.

As about the size. I mentioned - using better tools for bundling it gives significantly better results. And i'm not saying that you should change the flow or scripts or mind thinking.

It gives better results when some of the consumers of that package use more better tooling. They won't use your bundles (which in anyway are not included anywhere), they will resolve the raw index.

wooorm added a commit that referenced this pull request Feb 9, 2017
@wooorm
Copy link
Member

wooorm commented Feb 9, 2017

Readability... It is absolutely clear what is what. More vars is just a noise.

Maybe that’s too subjective, but I like my readability


Anyway, I took a stab at what I think is still very readable, and creates a smaller size.
This change I just committed (0369e03) removes 31 bytes Gzipped.
It’s 20 less than your commit, but it trades that for still being very performant and readable (IMO).

What do you think?

👋

@tunnckoCore
Copy link
Author

Still don't like it and that depth and props - it can be just node in top and directly write to node.properties.id or node.tagName.

But anyway.

@wooorm
Copy link
Member

wooorm commented Feb 9, 2017

Dude, that just increases the size...

@tunnckoCore
Copy link
Author

Not always. It depends on the way the whole code is written.

@tunnckoCore
Copy link
Author

And specifically in that case node.tagName would become something like that n.t=s instead of var n='div' and later n=s (name = subvalue)

@wooorm wooorm changed the title smaller size Improve bundle size Aug 11, 2019
@wooorm wooorm added ⛵️ status/released 🏁 area/perf This affects performance 👶 semver/patch This is a backwards-compatible fix 🦋 type/enhancement This is great to have labels Aug 11, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 area/perf This affects performance 💪 phase/solved Post is done 👶 semver/patch This is a backwards-compatible fix 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

2 participants