Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Closed
wants to merge 1 commit into from
+4,317 −5,567

4 participants

@mikesherov
Collaborator

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

@jaubourg
Collaborator

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

@mikesherov
Collaborator

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
Collaborator

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
Collaborator

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.

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 Collaborator

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

@mikesherov Collaborator

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

@timmywil Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mikesherov
Collaborator

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

@mikesherov
Collaborator
@timmywil
Collaborator

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
Collaborator
@mikesherov
Collaborator

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
Collaborator

@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
Owner
@timmywil
Collaborator

@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 dmethvin closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 7, 2011
  1. @mikesherov

    getting JSHint to work the same as our current JSLint works, fixes #1…

    mikesherov authored
    …0692.
    
    also, still needs to wait on this sizzle PR: jquery/sizzle#82
Something went wrong with that request. Please try again.