JSLint Reports on last 2 Production Versions of jQuery
in [JavaScript]
|
Prev: FAQ Entry Proposal: What is (function(){ /*...*/ })() ?
Next: FAQ Topic - What is Ajax? (2010-03-01)
From: lorlarz on 1 Mar 2010 22:57 On Mar 1, 6:56 pm, Stefan Weiss <krewech...(a)gmail.com> wrote: > On 02/03/10 01:32, lorlarz wrote: > > > ** I re-ran jQuery in JSLint and nothing WHATSOEVER CHANGED IN THE > > REPORT *** > > (i.e exactly the SAME long list of 'errors' was generated by JSLint.) > > I didn't try it myself, but I think that's highly unlikely. I only > clicked on one of the changeset links you posted, and in that set alone, > at least 20-30 JSLint warnings should have been fixed. Did you really > check out the current trunk version of JQuery? HEAD, or whatever it's > called in Git-land (sorry, svn user here). > [snip] > > -- > stefan ** BIG UPDATE **: jQuery now passes JSLint 100% but with noted exceptions: Quoting a recent post from John Resig to the group http://forum.jquery.com/developing-jquery-core Quoting it in-full: An update: jQuery is now 100% passing JSLint (with a few exceptions, noted below) and is integrated into the jQuery build process. I've been making changes to the codebase all day today and it's at a good state now. Here is where I integrated JSLint into the jQuery build process: http://github.com/jquery/jquery/commit/950b5d64a27994db1697eb4e605f5ea48ad8021b A list of the exceptions that we ignore in our JSLint run can be found here, with explanation: http://docs.jquery.com/JQuery_Core_Style_Guidelines#JSLint (end quote)
From: Stefan Weiss on 2 Mar 2010 00:24 On 02/03/10 04:57, lorlarz wrote: > ** BIG UPDATE **: > jQuery now passes JSLint 100% but with noted exceptions: That's good news. > Quoting a recent post from John Resig to the > group http://forum.jquery.com/developing-jquery-core > > Quoting it in-full: > > An update: jQuery is now 100% passing JSLint (with a few exceptions, > noted below) and is integrated into the jQuery build process. I've > been making changes to the codebase all day today and it's at a good > state now. > > Here is where I integrated JSLint into the jQuery build process: > http://github.com/jquery/jquery/commit/950b5d64a27994db1697eb4e605f5ea48ad8021b > > A list of the exceptions that we ignore in our JSLint run can be found > here, with explanation: > http://docs.jquery.com/JQuery_Core_Style_Guidelines#JSLint | We ignore the following warnings from JSLint: | | * "Expected an identifier and instead saw 'undefined' (a reserved | word)." In jQuery we re-declare 'undefined' as we use it internally. | Doing so prevents users from accidentally munging the name (assigning | to undefined and messing up our tests) and also makes the undefined | checks slightly faster. It has been mentioned on the JSLint mailing list that the text of this warning is confusing, because "undefined" is not, in fact, a reserved word. They could have used something like "undef" or "_undefined" in jQuery to avoid the warning. | * "Use '===' to compare with 'null'." and "Use '!==' to compare with | 'null'." - Almost universally in jQuery core we use === or !== with | the exception of when we're comparing against null. It's actually | quite useful to do == null or != null as it will pass (or fail) if the | value is either null or undefined. If we want to explicitly check for | a null or undefined value we use === null or === undefined (discussed | in depth below). Seems to happen a lot in jQuery. Funny, I never had a problem with this warning; I usually either check for "really not null", or truthy. Anything more complicated than that tends to end up in a function. | * "Expected an assignment or function call and instead saw an | expression." - There are two areas in which we access a property with | seemingly no side effect, those are: | | o parent.selectedIndex; (or similar) this is due to the fact that | WebKit mis-reports the default selected property of an option, | accessing the parent's selectedIndex property fixes it. David mentioned there was a better way to work around this, but I'm not sure what he was referring to. | o Array.prototype.slice.call( document.documentElement.childNodes, | 0)[0].node... - This check simply attempts to determine if, after a | slice on a NodeList, the resulting collection still holds DOM nodes. | This is a feature detection and is wrapped in a try/catch (if the | property doesn't exist it'll throw an exception. | | * "Expected a 'break' statement before 'case'." - There is only one | legitimate reason to use switch statements over an object of | properties or a series of if/else statements: passthroughs. We use it | inside of Sizzle to optimize our :only-child selector checks, reducing | code and improving performance. I suggested that JSLint should accept a /*fallthrough*/ comment here, but Douglas Crockford didn't agree. | Additionally we enable the following two JSLint options by default: | | * "evil" - This allows code evaluation. We only use it in one place | inside of jQuery: Backwards compatibility support for JSON parsing (we | preferably use the modern JSON.parse method if it's available). | | * "forin" - This removes the requirement of doing a hasOwnProperty | check inside of for/in loops. jQuery doesn't support working in an | environment that has manipulated the Object.prototype as it's | considered harmful. Both of these sound reasonable. I used to use hasOwnProperty() religiously, but now I think that users who augment Object.prototype deserve what they get. Which means that neither jQuery nor my code will work alongside Prototype.js; I don't have a problem with that. -- stefan
From: David Mark on 2 Mar 2010 00:34 lorlarz wrote: > On Mar 1, 6:56 pm, Stefan Weiss <krewech...(a)gmail.com> wrote: >> On 02/03/10 01:32, lorlarz wrote: >> >>> ** I re-ran jQuery in JSLint and nothing WHATSOEVER CHANGED IN THE >>> REPORT *** >>> (i.e exactly the SAME long list of 'errors' was generated by JSLint.) >> I didn't try it myself, but I think that's highly unlikely. I only >> clicked on one of the changeset links you posted, and in that set alone, >> at least 20-30 JSLint warnings should have been fixed. Did you really >> check out the current trunk version of JQuery? HEAD, or whatever it's >> called in Git-land (sorry, svn user here). >> > [snip] >> -- >> stefan > > ** BIG UPDATE **: > jQuery now passes JSLint 100% but with noted exceptions: > > Quoting a recent post from John Resig to the > group http://forum.jquery.com/developing-jquery-core > > Quoting it in-full: > > An update: jQuery is now 100% passing JSLint (with a few exceptions, > noted below) He really led off with that? > and is integrated into the jQuery build process. I've > been making changes to the codebase all day today and it's at a good > state now. Yes, I'm sure he was anxious to prove me wrong about his willingness to make changes (and these were very easy ones of course). No need to thank me. But, as pointed out, this changes very little in terms of the viability of the script. In fact, it literally changes nothing for sites that are running on pre-epiphany jQuery. And it took all day?! :) > > Here is where I integrated JSLint into the jQuery build process: > http://github.com/jquery/jquery/commit/950b5d64a27994db1697eb4e605f5ea48ad8021b > > A list of the exceptions that we ignore in our JSLint run can be found > here, with explanation: > http://docs.jquery.com/JQuery_Core_Style_Guidelines#JSLint "We ignore the following warnings from JSLint: "Expected an identifier and instead saw 'undefined' (a reserved word)." - In jQuery we re-declare 'undefined' as we use it internally. Doing so prevents users from accidentally munging the name (assigning to undefined and messing up our tests) and also makes the undefined checks slightly faster." Messing up their tests?! And I fail to see how using a variable makes checking for an undefined variable any faster. ""Use '===' to compare with 'null'." and "Use '!==' to compare with 'null'." - Almost universally in jQuery core we use === or !== with the exception of when we're comparing against null. It's actually quite useful to do == null or != null as it will pass (or fail) if the value is either null or undefined. If we want to explicitly check for a null or undefined value we use === null or === undefined (discussed in depth below)." Of course, that was a large percentage of the warnings. And how is it "quite useful" to write hopelessly ambiguous code? They use that in lots of places where it shouldn't be expected to match more than one of those values. ""Expected an assignment or function call and instead saw an expression." - There are two areas in which we access a property with seemingly no side effect, those are: parent.selectedIndex; (or similar) this is due to the fact that WebKit mis-reports the default selected property of an option, accessing the parent's selectedIndex property fixes it." We went over (and over) that. It's a ridiculous incantation that is easily avoided. Just check the selectedIndex property as it has the information needed! Don't evaluate it, pray that that has the desired side effect and then check a different property. If there is one (two as it is duplicated) "area" that demonstrates the failings of John Resig to understand basic browser scripting, this is it. He bases decisions on what he can see happening (today) rather than sound logic. This leads to scripts that "work" today and fail tomorrow. It's hardly unexpected that he would choose to ignore this (again). "Array.prototype.slice.call( document.documentElement.childNodes, 0 )[0].node... - This check simply attempts to determine if, after a slice on a NodeList, the resulting collection still holds DOM nodes. This is a feature detection and is wrapped in a try/catch (if the property doesn't exist it'll throw an exception." And, that can hardly be what JSLint was complaining about as it isn't concerned with the code's _logic_ (or host objects). ""Expected a 'break' statement before 'case'." - There is only one legitimate reason to use switch statements over an object of properties or a series of if/else statements: passthroughs." That's meaningless gobbledygook. Who decided that switch statements without "pass-throughs" were illegitimate? On the contrary, pass-throughs are exactly what JSLint is (legitimately) complaining about as they require careful examination to determine whether the lack of a - hreak - was intended or an oversight. And what the hell is an "object of properties?" ""We use it inside of Sizzle to optimize our :only-child selector checks, reducing code and improving performance." Whatever. "Additionally we enable the following two JSLint options by default: "evil" - This allows code evaluation. We only use it in one place inside of jQuery: Backwards compatibility support for JSON parsing (we preferably use the modern JSON.parse method if it's available)." Evil is right. IIRC, they never did figure out that the Function constructor is more appropriate for that task. ""forin" - This removes the requirement of doing a hasOwnProperty check inside of for/in loops. jQuery doesn't support working in an environment that has manipulated the Object.prototype as it's considered harmful." Yes, harmful to incompetently written scripts. ;) The style guide continues:- "TYPE CHECKS String: typeof object === "string" Number: typeof object === "number" Boolean: typeof object === "boolean" Object: typeof object === "object"" Cargo cult. There's no reason to use strict comparisons here. "Function: jQuery.isFunction(object)" typeof fn == 'function'; // Don't ever use such tests with host objects "Array: jQuery.isArray(object)" I'm sure that's the less-than-solid "Miller Device" (another device that should be designed out of the system, but can't be at this late date). "Element: object.nodeType null: object === null null or undefined: object == null" That last one is used to death in jQuery, yet it would seem to be (and should be) rarely needed. "undefined: Global Variables: typeof variable === "undefined" Local Variables: variable === undefined Properties: object.prop === undefined" Ridiculously counter-intuitive (and this is because of their undefined argument trick). "REGEXP All RegExp operations should be done using .test() and .exec(). "string".match() is no longer used." Why? IIRC, they used to use - match - where - test - would be more appropriate (as I pointed out in numerous reviews), but that is hardly a style issue as they do two different things. "STRINGS Strings should always use double-quotes instead of single-quotes." Even if they contain double-quotes? :) "NODES ..nodeName should always be used in favor of .tagName." Those are hardly equivalent. This is the usual over-generalization that leads to would-be programmers shooting their toes off. ".nodeType should be used to determine the classification of a node (not ..nodeName)." Apples and oranges (again). "Don't attach expandos to nodes - only attach data using .data()." ....which adds an expando. :) When it comes to expandos, it's in for a penny, in for a pound. They should be avoided at design time, but it's a bit late for jQuery to change that aspect of their design. And due to their attr woes, at one point at least, these expandos were being manifested as custom attributes in IE8. (!) Admittedly, some modules in My Library (e.g. query) attach an expando to element nodes, but at least IE is spared (as of late), and IE is the only major browser known to throw exceptions on such assignments. I think there is still a check for document.expando === false guarding the query module, which I can now safely remove. There are also a few other places where document nodes receive expandos (and are guarded with similar checks that are no guarantee for non-IE browsers), but those will be removable once support for frames is relegated to builds that explicitly request such support (of course, frames-enabled builds will still require them).
From: David Mark on 2 Mar 2010 00:54 Stefan Weiss wrote: > On 02/03/10 04:57, lorlarz wrote: >> ** BIG UPDATE **: >> jQuery now passes JSLint 100% but with noted exceptions: > > That's good news. > >> Quoting a recent post from John Resig to the >> group http://forum.jquery.com/developing-jquery-core >> >> Quoting it in-full: >> >> An update: jQuery is now 100% passing JSLint (with a few exceptions, >> noted below) and is integrated into the jQuery build process. I've >> been making changes to the codebase all day today and it's at a good >> state now. >> >> Here is where I integrated JSLint into the jQuery build process: >> http://github.com/jquery/jquery/commit/950b5d64a27994db1697eb4e605f5ea48ad8021b >> >> A list of the exceptions that we ignore in our JSLint run can be found >> here, with explanation: >> http://docs.jquery.com/JQuery_Core_Style_Guidelines#JSLint > > | We ignore the following warnings from JSLint: > | > | * "Expected an identifier and instead saw 'undefined' (a reserved > | word)." In jQuery we re-declare 'undefined' as we use it internally. > | Doing so prevents users from accidentally munging the name (assigning > | to undefined and messing up our tests) and also makes the undefined > | checks slightly faster. > > It has been mentioned on the JSLint mailing list that the text of this > warning is confusing, because "undefined" is not, in fact, a reserved > word. They could have used something like "undef" or "_undefined" in > jQuery to avoid the warning. Yes, that would be one sane approach. > > | * "Use '===' to compare with 'null'." and "Use '!==' to compare with > | 'null'." - Almost universally in jQuery core we use === or !== with > | the exception of when we're comparing against null. It's actually > | quite useful to do == null or != null as it will pass (or fail) if the > | value is either null or undefined. If we want to explicitly check for > | a null or undefined value we use === null or === undefined (discussed > | in depth below). > > Seems to happen a lot in jQuery. Indeed. It was the bulk of the warnings IIRC. > Funny, I never had a problem with this > warning; I usually either check for "really not null", or truthy. Sure, it depends on the context. > Anything more complicated than that tends to end up in a function. They seem to favor ambiguity over readability. > > | * "Expected an assignment or function call and instead saw an > | expression." - There are two areas in which we access a property with > | seemingly no side effect, those are: > | > | o parent.selectedIndex; (or similar) this is due to the fact that > | WebKit mis-reports the default selected property of an option, > | accessing the parent's selectedIndex property fixes it. > > David mentioned there was a better way to work around this, but I'm not > sure what he was referring to. The "issue" is that prior to the DOM being ready (or for SELECT's that have yet to be appended to the DOM), in some browsers, the - selected - property of the first contained OPTION element will not be set _unless_ it has a SELECTED attribute (which would only be an issue if _none_ of the other options is selected, either by attribute or memorized user intervention). Of course, at least one OPTION should have a SELECTED attribute. But regardless, the selectedIndex property invariably reveals which option is selected, so it is trivial to determine if a random OPTION is selected or not by checking the value of that property. This was explained to Resig (by proxy of course) and then re-explained (at least one time). Somehow he decided that his mystical incantation was more "concise" (meaning less _lines_ of code) and therefore it remains today (four times over at this point due to duplication and the possibility of the parentNode being an OPTGROUP). It's funny that even JSLint recognized it as silly and he deflected its advice as well. Hopeless. :( > > | o Array.prototype.slice.call( document.documentElement.childNodes, > | 0)[0].node... - This check simply attempts to determine if, after a > | slice on a NodeList, the resulting collection still holds DOM nodes. > | This is a feature detection and is wrapped in a try/catch (if the > | property doesn't exist it'll throw an exception. > | > | * "Expected a 'break' statement before 'case'." - There is only one > | legitimate reason to use switch statements over an object of > | properties or a series of if/else statements: passthroughs. We use it > | inside of Sizzle to optimize our :only-child selector checks, reducing > | code and improving performance. > > I suggested that JSLint should accept a /*fallthrough*/ comment here, > but Douglas Crockford didn't agree. I can see such a comment as a suitable substitute. Still, as I noted in my review of these notes, Resig's assertions about switch statements are ludicrous at best (I've read them three times now and still have no idea what he's even talking about). > > | Additionally we enable the following two JSLint options by default: > | > | * "evil" - This allows code evaluation. We only use it in one place > | inside of jQuery: Backwards compatibility support for JSON parsing (we > | preferably use the modern JSON.parse method if it's available). > | > | * "forin" - This removes the requirement of doing a hasOwnProperty > | check inside of for/in loops. jQuery doesn't support working in an > | environment that has manipulated the Object.prototype as it's > | considered harmful. > > Both of these sound reasonable. I used to use hasOwnProperty() > religiously, but now I think that users who augment Object.prototype > deserve what they get. But if your scripts must coexist with others on the Web... > Which means that neither jQuery nor my code will > work alongside Prototype.js; I don't have a problem with that. > > Ah, I think they finally fixed those problems (after years of broken scripts and public ridicule). Of course, the old versions are likely still floating around out there.
From: "Michael Haufe ("TNO")" on 2 Mar 2010 01:07
On Mar 1, 11:34 pm, David Mark <dmark.cins...(a)gmail.com> wrote: > "Expected an identifier and instead saw 'undefined' (a reserved word)." > - In jQuery we re-declare 'undefined' as we use it internally. Doing so > prevents users from accidentally munging the name (assigning to > undefined and messing up our tests) and also makes the undefined checks > slightly faster." > > Messing up their tests?! And I fail to see how using a variable makes > checking for an undefined variable any faster. I would expect it to be significantly faster and more correct if they used "void 0" instead of a name of any sort for this. Of course, no one there reads this list by their own admission... So I wouldn't be surprised to see this exact change shows up in their code base soon ;). |