Skip to content

Discussion around Zepto Style Guides #623

Closed
shalecraig opened this Issue Oct 22, 2012 · 5 comments

3 participants

@shalecraig

I intend for this to be a discussion about Zepto style guides, as I've been reading through the code and have a few questions about the way Zepto is implemented.

  1. Why is it important for Zepto not to use semicolons? I understand that it's possible for a library to be written like this, but it's generally frowned upon in the js community to write code without semicolons. We are using a js compiler (uglifier), so we don't need to worry about brevity.

  2. Why do we abuse ternary operators? In files such as src/zepto.js (deserializeValue, for example) we use at least three ternary operators in a row, instead of an if statement. I understand it feels clever to make changes to the codebase so it is smaller, but we are using a js compiler (uglifier), which will hopefully do it for us anyway. Even more, functions like zepto.qsa are needlessly hard to debug.

    // zepto.qsa
      zepto.qsa = function(element, selector){
        var found
        return (element === document && idSelectorRE.test(selector)) ?
          ( (found = element.getElementById(RegExp.$1)) ? [found] : [] ) :
          (element.nodeType !== 1 && element.nodeType !== 9) ? [] :
          slice.call(
            classSelectorRE.test(selector) ? element.getElementsByClassName(RegExp.$1) :
            tagSelectorRE.test(selector) ? element.getElementsByTagName(selector) :
            element.querySelectorAll(selector)
          )
      }
  3. Why don't we use static analysis tools like JS(hint or lint) to detect errors in our code? Of course we're all very smart and stuff, but computers are diligent.

  4. Has anybody done extensive testing to check if using Uglify.js is the best compiler to use? I've tried closure compiler in the past, and it has dead code elimination, and complains if you write stupid code. (awesome)

Zepto's website makes the claim you can "Learn from Zepto's clear and concise source code on how your favourite methods are implemented." I personally wouldn't say Zepto's source code is clear. It's definitely concise.

@shalecraig

@mislav & @madrobby : I don't mean for this to be a pull request, but I've forked the library and changed a few things.

So far, here is what I've done:

  1. Added semicolons
  2. Ran it through js beautifier (consistent formatting is awesome, why not do it?)
  3. Ran it through jshint
  4. Added the closure compiler (along with uglify, for comparison)

The tests still pass, so I'm not worried about breaking functionality. If I've broken anything, let me know though.

I'm not going to devote much more time on my fork, but there are a few of the things that I figured out in the process:

  • Both Uglify and Closure compiler do a better job at minification when ternary operators are not used.
  • If Zepto was run through jshint, we would find > 10 variables that are set but are never used through the entire codebase. Not so great.
  • I have been able to minify Zepto.js to be under 9k gzipped, (with the default rake task) in all js compilers/uglifiers.

Here is the rake output from my fork:

$ rake
Original version: 55.289k
Uglified: 23.951k
Uglified then gzipped: 8.854k, compression factor 6.245
Closured: 23.548k
Closured then gzipped: 8.847k, compression factor 6.250
AClosure: 21.904k
AClosure then gzipped: 8.488k, compression factor 6.514

AClosure = Google Closure compiler with advanced optimizations.

PS: I will take pull requests, but this has probably moved too far away from master to be of use.

@aggsol
aggsol commented Oct 24, 2012

+1 I agree. Tools assistance is a good thing especially for dead code and unused variable. I also find the ternary operator hard to read, especially in someone's else code.

"Learn from Zepto's clear and concise source code on how your favourite methods are implemented." I personally wouldn't say Zepto's source code is clear. It's definitely concise.

This is so true.

@mislav
Collaborator
mislav commented Oct 24, 2012

Why is it important for Zepto not to use semicolons?

1) It's not important! It's just the way we like it. We don't care about what's "generally frowned upon" by the community; we write the code in a style that best suites us, and we have decided not to write needless characters.

Twitter Bootstrap JS components are written without semicolons and nobody is making a big deal out of it. It remains one of the most popular opensource projects on GitHub and, most importantly, it works.

Why do we abuse ternary operators?

2) Because it reduces size. The primary guideline for Zepto's style isn't clarity/readability (that's 2nd); it's small file size. Keywords like if and else are more bytes to write, and a minifier can't safely rewrite them (to my knowledge).

Nested ternary operators aren't hard, either. The statements get executed from left to right:

a ? b : c ? d : e ? f : g

However, you're welcome to try using conditional statements instead of ternary operators and we'll accept the refactoring if it doesn't change the code size too much. We've already done this sort of change on occassion.

Why don't we use static analysis tools like JS(hint or lint) to detect errors in our code?

3) Because we're all very smart and stuff :)

We have an automated test suite to detect errors in our code. The test suite is dilligent, and can actually verify that the code works as expected.

Has anybody done extensive testing to check if using Uglify.js is the best compiler to use?

4) We haven't, but I think that we switched to Uglify because it produced smaller code and was a little easier to integrate in our build system. However, your Aclosure results are interesting.

We don't need dead code elimination because we don't have dead code.

I personally wouldn't say Zepto's source code is clear.

I agree. I personally try to make it as clear as possible, but the size constraint is really limiting. Every time I refactor code for readability, I see the size blow up. That's why I can't let myself write code in the way I would write it if I wasn't constrained by size.

I'm going to close this because it contains too many changes that we won't accept. We would like you to submit smaller, more manageable changes that:

  • refactor the code to be of better quality while preserving small size
  • eliminate unused variables.
@mislav mislav closed this Oct 24, 2012
@shalecraig

A few points to address your response: (If I may re-open this)

  • Semicolons

It's not important! It's just the way we like it. We don't care about what's "generally frowned upon" by the community; we write the code in a style that best suites us, and we have decided not to write needless characters.
Twitter Bootstrap JS components are written without semicolons and nobody is making a big deal out of it. It remains one of the most popular opensource projects on GitHub and, most importantly, it works.

I understand you don't want to write needless characters, and recognize the size constraints that the library is trying to fit inside. If the reason that you don't want to write semicolons is because of the size, they are removed by Uglify.js. If the reason that you don't want to write semicolons is because you are smarter than that, I don't think it's a good move to invite collaborators that aren't as smart as you to contribute towards this project, and invites contributors to make mistakes that will break the library. I wouldn't claim we have 100% test coverage.

I don't care so much about bootstrap, but people have also made issues about this sort of stuff.

  • Ternary operators.

Because it reduces size. The primary guideline for Zepto's style isn't clarity/readability (that's 2nd); it's small file size. Keywords like if and else are more bytes to write, and a minifier can't safely rewrite them (to my knowledge).
Nested ternary operators aren't hard, either. The statements get executed from left to right:
a ? b : c ? d : e ? f : g
However, you're welcome to try using conditional statements instead of ternary operators and we'll accept the refactoring if it doesn't change the code size too much. We've already done this sort of change on occassion.

I understand you want to use ternary operators to keep the library small, but control flow logic (if/else/etc) is changed to ternary operators by Ugilfy.js and most other compilers. It really makes it harder for people to contribute.

While ternary operators are easy to write, and I understand very well how they work, they are harder to maintain and much harder for other programmers to understand the control flow/logic if it is separated over many lines.

  • Static Analysis tools.

Because we're all very smart and stuff :)
We have an automated test suite to detect errors in our code. The test suite is dilligent, and can actually verify that the code works as expected.

Automated test suites are cool, and they're good to have. Let's add to that net by automatically running jshint at the same time.

  • JS Compilers/Code Quality:

I agree. I personally try to make it as clear as possible, but the size constraint is really limiting.
Every time I refactor code for readability, I see the size blow up.
That's why I can't let myself write code in the way I would write it if I wasn't constrained by size.

I'm not sure if you're talking about the pre-compiled size constraint, or the min.js size constraint. The official min.js 14% bigger than my fork's min.js, under about any compiler I tested.

Without significant work, aClosure will not work for any use case. It renames almost every function, so we need to use exports. This might be a good goal, for Zepto 2.0.

Here are some fixes that I'll commit to doing:

  1. Pull request to add semicolons to the zepto.js main trunk. (And automatic semicolon insertion through an automatic tool run as a commit hook.)
  2. Pull request to add a style guide cautioning against nested ternary operators.
  3. Pull request to add a jslint step to running the test suite.
  4. Pull request to add closure as a secondary build option in a development branch of zepto.
  5. Learn enough ruby not to make myself look stupid when I change the rakefile.

How does this sound?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.