Skip to content

Loading…

make code grunt-contrib-jshint 0.3.0 compatible #1203

Closed
wants to merge 1 commit into from

5 participants

@mgol
jQuery Foundation member

No description provided.

@mgol
jQuery Foundation member

Note that I had to switch to false both the noempty option because of two while-s with empty blocks and the strict option because turning strict mode on has been reverted recently.

@rwaldron rwaldron commented on an outdated diff
src/ajax.js
@@ -444,8 +556,8 @@ jQuery.extend({
parts = rurl.exec( s.url.toLowerCase() );
s.crossDomain = !!( parts &&
( parts[ 1 ] !== ajaxLocParts[ 1 ] || parts[ 2 ] !== ajaxLocParts[ 2 ] ||
- ( parts[ 3 ] || ( parts[ 1 ] === "http:" ? 80 : 443 ) ) !=
- ( ajaxLocParts[ 3 ] || ( ajaxLocParts[ 1 ] === "http:" ? 80 : 443 ) ) )
+ ( parts[ 3 ] || ( parts[ 1 ] === "http:" ? "80" : "443" ) ) !==
+ ( ajaxLocParts[ 3 ] || ( ajaxLocParts[ 1 ] === "http:" ? "80" : "443" ) ) )
@rwaldron jQuery Foundation member
@gibson042 jQuery Foundation member

This one will work because it changes the numbers to strings, but will probably increase file size.

@mgol jQuery Foundation member
mgol added a note

It probably will but so does using triple = instead of double in most places and many such cases are not, strictly speaking, required for the code to work. And since this is one of very few places were !=/== exist and current .jshintrc configuration forbids it...

If you decide to not change it then it should at least be surrounded by jshint comment blocks disabling the eqeqeq option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rwaldron rwaldron commented on an outdated diff
src/core.js
@@ -404,7 +404,7 @@ jQuery.extend({
isArray: Array.isArray,
isWindow: function( obj ) {
- return obj != null && obj == obj.window;
+ return obj != null && obj === obj.window;
@rwaldron jQuery Foundation member

Did you test this in IE?

@gibson042 jQuery Foundation member

Both this change and the one in event.js will break oldIE: http://jsbin.com/eriruv/1

@mgol jQuery Foundation member
mgol added a note

Isn't breaking oldIE allowed in 2.0.x?

@gibson042 jQuery Foundation member

Yes, of course. My mistake for not checking the target branch.

@gibson042 jQuery Foundation member

That being said, though, I'm not really comfortable with introducing divergence in the development dependencies of 1.x and 2.x.

@mgol jQuery Foundation member
mgol added a note

Fair enough. BTW, what's the official way to get to the 1.9.x code base? Are commits from master manually cherry-picked by jQuery team members?

EDIT: I'll revert the change taking into account your last comment but I'll surround it by jhsint comments temporarily disabling the eqeqeq option so that current jsHint doesn't fail on this.

@gibson042 jQuery Foundation member

Pull requests can freely target either master or 1.9-stable, and we cherry pick as appropriate.

@rwaldron jQuery Foundation member

Yep, breaking oldIE is cool, I was only asking about IE (as in 9, 10)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rwaldron rwaldron commented on an outdated diff
.jshintrc
((11 lines not shown))
"trailing": true,
- "node": true
+ "eqnull": true,
+ "expr": true,
+ "boss": true,
+ "sub": true,
+
+ "node": true,
+ "browser": true,
+
+ "globals": {
+ "define": false
+ }
@rwaldron jQuery Foundation member

This is the .jshintrc in the root of the repo, right? That's used for Gruntfile.js. The one in the src/ is the one used for linting the source files.

@mgol jQuery Foundation member
mgol added a note

Ahh, that explains a lot. I'll look into it again tomorrow (I'm now away from my laptop).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gibson042
jQuery Foundation member

I personally find the code taking advantage of function hoisting to be more readable and better organized. It might be time to try undef:true and latedef:false, per jshint/jshint#424 (comment), which would eliminate the need for these large block moves.

@mgol
jQuery Foundation member

Ok, I'll check how it works tomorrow.

@mgol
jQuery Foundation member

I updated the pull request according to review comments; most changes were, indeed, not needed. It seems everything works fine after an update to grunt-contrib-jshint 0.2.0 now so I updated it in package.json, too.

@rwldrn & @gibson042: as for the !== window comparisons - all unit tests after my modifications pass on both IE9 & IE10. However, I was thinking: these != comparisons are still needed for 1.x and it will cause jsHint breakage so /*jshint comments will be needed there anyway. We can either keep 1.x & 2.x different here or change comparisons back to != and wrap them in those jsHint comments disabling eqeqeq for a moment.

@gibson042
jQuery Foundation member

It is appropriate to keep the strict this/window comparisons in 2.0. Could you open an analogous PR for 1.9-stable that instead uses // Support: IE<9 and /*jshint eqeqeq:false */?

@mgol
jQuery Foundation member

Sure, I'll do it within a few hours.

BTW, jsHint >= 1.0 used by this upgraded grunt-contrib-jshint allows using space between /* and jshint or global so maybe it would be better to style it like that?

/* jshint eqeqeq: false */

Seems more in line with jQuery liberal spacing policy.

@gibson042
jQuery Foundation member

Yeah, I like it.

@mgol
jQuery Foundation member

I cleaned up jsHint conf options: regexdash doesn't seem to take any effect (and it's not in docs), I also removed wsh (why was it needed? nothing in the code requires it), I added eqeqeq (it's not enforced by default and the code base adheres to it anyway) and es5 (fine for any browser supported by jQuery 2.0).

@mgol
jQuery Foundation member

Please note that for this code to build you must still use node 0.8.x, not 0.10.0 as long as pull request #1202 is not pulled in.

@mgol
jQuery Foundation member

Ah, I also removed the evil option - it seems dangerous and since it's not needed at all (it is still needed in 1.9-stable, though)...

@mgol
jQuery Foundation member

I created an analogous pull request for 1.9-stable: #1204

@mikesherov
jQuery Foundation member

We can always turn on eqeqeq and inline comment turn it off for the isWindow check in 1.9, although @timmywil was at one point against inline jshint commenting.

@mgol
jQuery Foundation member

That's exactly what I did in #1204. Of course, another option is to turn off eqeqeq in 1.9-stable but since it's needed only in 3 places and it's a good practice to not use == if not needed...

@mgol
jQuery Foundation member

Rebased to master; thanks to that it now correctly builds on node 0.10.0 so it's easier to test it.

Also, restored the regexdash option as it's still needed, though not documented on jshint.com/docs for some reason.

@mgol
jQuery Foundation member

BTW, if/when you merge this one I'll rebase all my other pull requests so that they can be checked against the newest jsHint version.

@mgol
jQuery Foundation member

Re-based to master and corrected some recently added leading white spaces which made grunt fail after re-basing.

Any chance of merging it soon so that new such mistakes aren't introduced?

@mgol
jQuery Foundation member

I made analogous updates to pull request #1204

@mgol
jQuery Foundation member

Re-based to master. Most of changes from this pull request has been already merged in commit 1205103 but a few remain.

@mgol
jQuery Foundation member

Github's way of displaying tabs is so broken... I've changed 4 spaces to 2 tabs here.

@mgol
jQuery Foundation member

This was needed only for ActiveX parts which are now gone.

@mgol
jQuery Foundation member

Nothing really uses this now but this relaxes some non-ES5 browsers rules that are no longer needed in IE9 and modern browsers.

@dmethvin
jQuery Foundation member

Somehow I missed this and cherry-picked gh-1204 to get the 1.x changes. Do these look good? 1205103 If so just close this one.

@mgol
jQuery Foundation member

@dmethvin, I've just rebased this one to master so that only the remaining parts are to be merged. There are only a few differences related mostly to comparisons with window which required == instead === in oldIE but are fine here. It was already discussed and both @rwldrn and @gibson042 agreed it's fine.

@mgol
jQuery Foundation member

I've just run the full unit test suite from this branch on IE9 and are all passed.

@mgol
jQuery Foundation member

Well, the commit description on the rebased one is now a little outdated but you can change it when merging anyway.

@mgol
jQuery Foundation member

AFAIK it was used only in globalEval in 1.x. It's probably better to remove it when it's not needed.

@dmethvin dmethvin closed this in ba16ba2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 4, 2013
  1. @mgol
This page is out of date. Refresh to see the latest.
Showing with 5 additions and 9 deletions.
  1. +1 −1 Gruntfile.js
  2. +1 −2 src/.jshintrc
  3. +1 −2 src/core.js
  4. +1 −1 src/effects.js
  5. +1 −3 src/event.js
View
2 Gruntfile.js
@@ -7,7 +7,7 @@ module.exports = function( grunt ) {
"dist/jquery.min.map",
"dist/jquery.min.js"
],
- gzip = require("gzip-js"),
+ gzip = require("gzip-js"),
readOptionalJSON = function( filepath ) {
var data = {};
try {
View
3 src/.jshintrc
@@ -11,12 +11,11 @@
"maxerr": 100,
"eqnull": true,
- "evil": true,
"sub": true,
"boss": true,
"browser": true,
- "wsh": true,
+ "es5": true,
"globals": {
"jQuery": true,
View
3 src/core.js
@@ -404,8 +404,7 @@ jQuery.extend({
isArray: Array.isArray,
isWindow: function( obj ) {
- /* jshint eqeqeq: false */
- return obj != null && obj == obj.window;
+ return obj != null && obj === obj.window;
},
isNumeric: function( obj ) {
View
2 src/effects.js
@@ -242,7 +242,7 @@ jQuery.Animation = jQuery.extend( Animation, {
});
function defaultPrefilter( elem, props, opts ) {
- /*jshint validthis:true */
+ /* jshint validthis: true */
var index, prop, value, length, dataShow, toggle, tween, hooks, oldfire,
anim = this,
style = elem.style,
View
4 src/event.js
@@ -402,9 +402,7 @@ jQuery.event = {
// Avoid non-left-click bubbling in Firefox (#3861)
if ( delegateCount && cur.nodeType && (!event.button || event.type !== "click") ) {
- /* jshint eqeqeq: false */
- for ( ; cur != this; cur = cur.parentNode || this ) {
- /* jshint eqeqeq: true */
+ for ( ; cur !== this; cur = cur.parentNode || this ) {
// Don't process clicks on disabled elements (#6911, #8165, #11382, #11764)
if ( cur.disabled !== true || event.type !== "click" ) {
Something went wrong with that request. Please try again.