inline replacement of JSLint with JSHint, fixes #10692 #602

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@mikesherov
Member

We'll move to undef:true and onevar:true later, MAYBE.

@jaubourg
Member

Why on Earth do we have that many changes? Some of these re-arranging do not seem to make a lot of sense.

@mikesherov
Member

jaubourg, I included pull 602 and 601 in case we weren't prepared for the undef changes.

The reason we need to rearrange is that static code analyzer need named functions to be declared before they are used. They also require vars to be at least declared as such before they are used.

If we DON'T want to check that all of our variables are defined before we use them, then undef: true is not for us. However, without it, nothing is going to tell you when you accidentally forget to declare a var inside a closure and it's promoted to global scope. I know @dmethvin recently had a bug with events that would have been avoided if we were using undef:true.

The tradeoff is there for evaluation.

@mikesherov
Member

And if you think these changes are superfluous, just wait till you see what I have to do to get onevar: true to pass. ;-)

@timmywil
Member

I knew this would require a lot of changes, but I'm ok with that. What is the size difference after this change? Is there any? uglify hoists anyway so I'm not sure it would actually make a difference.

@timmywil timmywil and 1 other commented on an outdated diff Nov 16, 2011
src/exports.js
@@ -15,6 +15,7 @@ window.jQuery = window.$ = jQuery;
// file names, and jQuery is normally delivered in a lowercase file name.
// Do this after creating the global so that if an AMD module wants to call
// noConflict to hide this version of jQuery, it will work.
+/*global define:true */
@timmywil
timmywil Nov 16, 2011 jQuery member

Let's not include comments like this. We can add define to the predef list in jshint-check

@mikesherov
mikesherov Nov 16, 2011 jQuery member

OK, but this is the way in code to indicate without having to modify the lint check. But I'm fine either way.

@timmywil
timmywil Nov 16, 2011 jQuery member

I think it's better to modify the lint check than to include non-contextual comments in the code.

@mikesherov
Member

@timmywil, I'll get a size diff shortly.

@mikesherov
Member

Yeah, I guess I see it as contextual and relevant. It says: we know
that "define" is a global var we're expecting.

Anyway, I'll defer to your preference. Do you also want me to move
ActiveXObject global defines to JSHint-check.js as well?

Sent Via Mobile: Please excuse my grammar, tone, and punctuation. My
thumbs can't create flowery prose.

On Nov 16, 2011, at 10:54 AM, timmywil
reply@reply.github.com
wrote:

I knew this would require a lot of changes, but I'm ok with that. What is the size difference after this change? Is there any? uglify hoists anyway so I'm not sure it would actually make a difference.


Reply to this email directly or view it on GitHub:
#602 (comment)

@timmywil
Member

My thinking:

  • I think a contextual comment would be
    // define is considered a global here
    Or better yet, the comment right above where burke explains the purpose of the define.
    /* global define:true */
    is a strictly formatted comment for jshint and we probably wouldn't think to add a comment saying something like that if jshint wasn't in the library.
  • I'd rather have everything related to jshint in one place, mainly for maintenance.

I would remove any jshint comments that have been added and instead adjust the options.

@mikesherov
Member

Fair enough. I'll make those changes and give a new size diff soon
when I get in front of a computer.

Sent Via Mobile: Please excuse my grammar, tone, and punctuation. My
thumbs can't create flowery prose.

On Nov 16, 2011, at 8:18 PM, timmywil
reply@reply.github.com
wrote:

My thinking:

  • I think a contextual comment would be
    // define is considered a global here
    Or better yet, the comment right above where burke explains the purpose of the define.
    /* global define:true */
    is a strictly formatted comment for jshint and we probably wouldn't think to add a comment saying something like that if jshint wasn't in the library.
  • I'd rather have everything related to jshint in one place, mainly for maintenance.

I would remove any jshint comments that have been added and instead adjust the options.


Reply to this email directly or view it on GitHub:
#602 (comment)

@mikesherov
Member

OK, so made those changes, and just ran a new size diff. Want to laugh?

jQuery Size - compared to last make
249437 (+19) jquery.js
94126 (+4) jquery.min.js
33244 (-26) jquery.min.js.gz

@mikesherov
Member

@timmywil, after those commits I just made, the only special cases in jshint-check.js are:

var ok = {
    "Expected a 'break' statement before 'case'.": true,
    "'DOMParser' is not defined.": true,
    "'define' is not defined.": true
};

So, I understand you don't want the global-style comments in code, but as far as expecting a break before case, there is a JSHint comment called: /* falls through */ that you put right before the fall through to indicate that you know you're falling through: jshint/jshint#18

The reason why I think this is different from the global one, is that in the case of globals, you're right, you can check on a case by case basis the global variable being assumed and ignore it, but if we have a blanket ignore on switch statement fall throughs, we'll won't be able to automate the catching of that mistake.

What do you think?

@dmethvin
Member

When I am writing C code I use a /* falls through */ comment, it's a
pretty common way to let people know you meant to do it.
I only see one switch in the entire codebase and it's in Sizzle, and
it has one fall-through. Didn't rwaldron mention something about
switch being really bad for perf in Chrome anyway? It wouldn't be too
hard to rewrite.

@timmywil
Member

@mikesherov: I'm fine with /* falls through */. As for the special cases, I didn't mean add to the accepted errors. For jshint, we can define a variable in the options called predef, which is an array of variable names that are expected to be global. I keep a list in my global .jshintrc:

"predef": [
    "define",
    "require",
    "jQuery",
    "YAHOO",
    "_gaq",
    "Modernizr"
]

So I imagine something like this in the jshint-check.

@dmethvin: rwaldron and I already looked at that switch. I think it's best to leave it and add the comment. The codepath is not actually used in Chrome so perf is not an issue.

@dmethvin
Member
dmethvin commented Dec 9, 2011
@dmethvin dmethvin closed this Dec 9, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment