set local scope of document

LeaVerou / multirange

A tiny polyfill for HTML5 multi-handle sliders

http://projects.verou.me/multirange/

MIT License

607 stars 82 forks source link

set local scope of document #9

Closed marcobiedermann closed 8 years ago

marcobiedermann commented 8 years ago

Caching the document locally will improve the performance and helps when minification is used.

LeaVerou commented 8 years ago

Hello there, thank you for contributing!

You realize there are three (3) uses of document in the code and that it's a 1KB script, right? What minification?! What performance improvement?!

I respect your effort and hate closing PRs without merging, but I think this is textbook premature optimization. I hope this does not discourage you from future contributions.

marcobiedermann commented 8 years ago

@LeaVerou

You are absolutely right in every point. Still I think that caching the document is an improvement. A really small one but it is :) I like to improve code, even if it is just a tiny bit. Lot's of tiny things make up a big one.

But I absolutely understand your point. That is totally fine :)

Thanks for this project and all your other great stuff 👍

LeaVerou commented 8 years ago

Still I think that caching the document is an improvement. A really small one but it is :)

For the sake of argument, I disagree. Everything comes at a cost. If document was used in tons of places, in loops etc, the cost would be worth it, but the cost is always there. For example, the cost of this is:

The pattern puzzles anyone who hasn't seen it before, making contributions harder.
It increases unminified code size by a bit, which means more code for humans to process.

Sure, it's a small cost, but it's also a small benefit, and I wouldn't be so sure which one outweighs the other.

marcobiedermann commented 8 years ago

@LeaVerou Alright fair enough! :)