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

Redundant variables (coding style) #269

Closed
mustafa0x opened this issue Feb 2, 2019 · 15 comments
Closed

Redundant variables (coding style) #269

mustafa0x opened this issue Feb 2, 2019 · 15 comments
Labels
invalid not related or critical

Comments

@mustafa0x
Copy link

I've noticed a lot of cases of x = 'x' in the code (e.g. backdrop = 'backdrop'), and couldn't figure out what the use of such a coding style is. I also found that it made the code harder to understand and debug, but could be a matter of taste.

Thanks.

@thednp thednp added the invalid not related or critical label Feb 3, 2019
@thednp
Copy link
Owner

thednp commented Feb 3, 2019

That's a better way to compress strings.

@thednp thednp closed this as completed Feb 3, 2019
@mustafa0x
Copy link
Author

mustafa0x commented Feb 3, 2019 via email

@thednp
Copy link
Owner

thednp commented Feb 3, 2019

If your file is properly built it should be ok.

@mustafa0x
Copy link
Author

mustafa0x commented Feb 3, 2019 via email

@thednp
Copy link
Owner

thednp commented Feb 3, 2019

I don't know I didn't try, but I think the size will become even smaller gzipped.

UPDATE: it's 7.65Kb gzipped, so less than half the size of the minified file.

@mustafa0x
Copy link
Author

Sorry, I was curious whether the compression improvement you mentioned also applies if the file is gzipped.

I'm actually curious in any case in which such a style improves string compression. Especially as I've been trying to reproduce the improvement but couldn't.

@thednp
Copy link
Owner

thednp commented Feb 3, 2019

You can do this test yourself I don't have the time to do this. From 70Kb unminified to 7.65Kb minified and gzipped, I think that's a huge deal, right?

@mustafa0x
Copy link
Author

I no longer use BSN in my projects, but came across something relevant to this discussion, and thought I'd share in case it's found beneficial:

mishoo/UglifyJS#185 (comment)

For now, it seems that improvements are unlikely due to gzip's more efficient string tokenization (gzip's pointers are smaller byte-wise that we could write in javascript), and any improvement would be marginal.

@thednp
Copy link
Owner

thednp commented Mar 9, 2019

I don't understand what would be the reason. but thanks for bringing that up. Again the main idea is to have something we have control over before gzip or other scripts do their stuff.

@mustafa0x
Copy link
Author

AFAIK it's because gzip (i.e. deflate) compresses recurring strings very efficiently, so it's pretty difficult to do a better job than it.

@thednp
Copy link
Owner

thednp commented Mar 9, 2019

So did you test yourself?

@mustafa0x
Copy link
Author

Here's a small sample:

Minified with tokenizing:

(function(){var n=document,e=n.documentElement,i="style",t="toLowerCase",o="Transition",a="Duration",d="Webkit",r=d+o in e[i]||o[t]()in e[i],u=d+o in e[i]?d[t]()+o+"End":o[t]()+"end",c=d+a in e[i]?d[t]()+o+a:o[t]()+a})();
  • 221 bytes, 175 bytes gzipped.

Minified without tokenizing:

(function(){var n=document,t=n.documentElement,i="WebkitTransition"in t.style||"Transition".toLowerCase()in t.style,e="WebkitTransition"in t.style?"Webkit".toLowerCase()+"TransitionEnd":"Transition".toLowerCase()+"end",o="WebkitDuration"in t.style?"Webkit".toLowerCase()+"TransitionDuration":"Transition".toLowerCase()+"Duration"})();
  • 334 bytes, 163 bytes gzipped.

So yes, this very basic sample supports that even though the latter has more characters, it ends up being smaller gzipped. Thus, manual tokenizing isn't worth it — the compressor does a better job than us puny humans. =]

@thednp
Copy link
Owner

thednp commented Mar 10, 2019

That is interesting and strong argument indeed and I thank you for your time.

Around 7% deficit on manual tokens vs gzip, my library minified is very small compared to original plugins and jQuery all together. It's hard to want to use something similar with larger size.

@thednp
Copy link
Owner

thednp commented Mar 14, 2019

@mustafa0x I've been thinking about this lately, here is my thinking. I won't have a problem reverting to the previous coding style, especially because I seriously consider rewriting the code in ES6 format and this manual tokenization is making it all likely more tedious.

Somehow I still wish 7zip/Gzip devs would take into account existing code tokenization before doing it's own. This would serve greatly to those massive chunks of folks who would like to have their script delivered in super minimized size but would rather not bother with server/cms/other settings to enable and configure compression.

@mustafa0x
Copy link
Author

gzip is supported everywhere and is almost always on by default, so I don't think optimizing for a non-gzip environment is that productive.

thednp added a commit that referenced this issue Dec 31, 2019
* added first ES6/ES7 code draft to `lib/src` folder, the following changes apply
* all components have `destroy()` method
* recent fixes to allow re-init are included (on re-init we trigger `destroy()` before anything else)
* more clear init objects ( in essence we're talking `{chaos,options,methods}` -> `{element,target,options,methods}` )
* removed manual tokenization, resulting in easier long term maintenance, the min files will be larger while gzip files should be 7-10% smaller, now @mustafa0x should be happy #269
* replaced `getClosest()` utility with native `element.closest(selector)`
* Popover and Tooltip now both use `options.template` and make use of attributes like `data-title` to fill in the template markup
* Popover and Tooltip can work simultaneously with other `data-toggle` components via `data-tip="tooltip"` and `data-tip="popover"` attributes (Modal,Dropdown,Tab,Collapse)

TO DO (priority ordered):
* debug and fix all issues resulted in removal of manual tokenization or other module import/link related issues
* evaluate and discuss code structure to find opportunities for new features and code quality improvements (constructor, private & public methods, etc)
* Toast component: aren't `destroy()` and `dispose()` methods supposed to do the same?
* we need a new ES6/ES7 friendly `initCallback`, we might also need an ES6/ES7 friendly `removeDataAPI` for all components, the opposite of `initCallback`, a possible draft at `src/util/callbacks.js`
* we need a new bundle builder (lib/bundle.js) to do ES6/ES7 `index.js` -> ES5 `bootstrap-native.js` compilation + uglify work, that's where YOU come in (rollup, babel, promise, whatever you need, bring it)
* we need a full documentation for V4 page, the future main page of the project
* wiki updates


@cvaize and everyone interested in this thread please join discussion here #306

Happy New Year!
thednp added a commit that referenced this issue Dec 31, 2019
* added first ES6/ES7 code draft to `lib/src` folder, the following changes apply
* all components have `destroy()` method
* recent fixes to allow re-init are included (on re-init we trigger `destroy()` before anything else)
* more clear init objects ( in essence we're talking `{chaos,options,methods}` -> `{element,target,options,methods}` )
* removed manual tokenization, resulting in easier long term maintenance, the min files will be larger while gzip files should be 7-10% smaller, now @mustafa0x should be happy #269
* replaced `getClosest()` utility with native `element.closest(selector)`
* Popover and Tooltip now both use `options.template` and make use of attributes like `data-title` to fill in the template markup
* Popover and Tooltip can work simultaneously with other `data-toggle` components via `data-tip="tooltip"` and `data-tip="popover"` attributes (Modal,Dropdown,Tab,Collapse)

TO DO (priority ordered):
* debug and fix all issues resulted in removal of manual tokenization or other module import/link related issues
* evaluate and discuss code structure to find opportunities for new features and code quality improvements (constructor, private & public methods, etc)
* Toast component: aren't `destroy()` and `dispose()` methods supposed to do the same?
* we need a new ES6/ES7 friendly `initCallback`, we might also need an ES6/ES7 friendly `removeDataAPI` for all components, the opposite of `initCallback`, a possible draft at `src/util/callbacks.js`
* we need a new bundle builder (lib/bundle.js) to do ES6/ES7 `index.js` -> ES5 `bootstrap-native.js` compilation + uglify work, that's where YOU come in (rollup, babel, promise, whatever you need, bring it)
* we need a full documentation for V4 page, the future main page of the project
* wiki updates


@cvaize and everyone interested in this thread please join discussion here #306

Happy New Year!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid not related or critical
Projects
None yet
Development

No branches or pull requests

2 participants