Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,5 @@
"root": true,
"env": {
"node": true
},
rules: {
// Until https://github.com/jquery/eslint-config-jquery/issues/7 is resolved
"no-unused-expressions": "error"
}
}
1 change: 1 addition & 0 deletions dist/.eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"no-multiple-empty-lines": "off",

// Because sizze is not compatible to jquery code style
"no-nested-ternary": "off",
"no-unused-expressions": "off",
"lines-around-comment": "off",
"space-in-parens": "off",
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"commitplease": "2.3.1",
"core-js": "2.2.2",
"cross-spawn": "2.2.3",
"eslint-config-jquery": "0.1.6",
"eslint-config-jquery": "1.0.0",
"grunt": "1.0.1",
"grunt-babel": "6.0.0",
"grunt-cli": "1.2.0",
Expand Down
17 changes: 12 additions & 5 deletions src/attributes/prop.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,19 @@ jQuery.extend( {
// Use proper attribute retrieval(#12072)
var tabindex = jQuery.find.attr( elem, "tabindex" );

return tabindex ?
parseInt( tabindex, 10 ) :
if ( tabindex ) {
return parseInt( tabindex, 10 );
}

if (
rfocusable.test( elem.nodeName ) ||
rclickable.test( elem.nodeName ) && elem.href ?
0 :
-1;
rclickable.test( elem.nodeName ) &&
elem.href
) {
return 0;
}

return -1;
}
}
},
Expand Down
26 changes: 15 additions & 11 deletions src/attributes/val.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ jQuery.fn.extend( {

ret = elem.value;

return typeof ret === "string" ?

// Handle most common string cases
ret.replace( rreturn, "" ) :
// Handle most common string cases
if ( typeof ret === "string" ) {
return ret.replace( rreturn, "" );
}

// Handle cases where value is null/undef or number
ret == null ? "" : ret;
// Handle cases where value is null/undef or number
return ret == null ? "" : ret;
}

return;
Expand Down Expand Up @@ -96,15 +96,19 @@ jQuery.extend( {
},
select: {
get: function( elem ) {
var value, option,
var value, option, i,
options = elem.options,
index = elem.selectedIndex,
one = elem.type === "select-one",
values = one ? null : [],
max = one ? index + 1 : options.length,
i = index < 0 ?
max :
one ? index : 0;
max = one ? index + 1 : options.length;

if ( index < 0 ) {
i = max;

} else {
i = one ? index : 0;
}

// Loop through all the selected options
for ( ; i < max; i++ ) {
Expand Down
11 changes: 6 additions & 5 deletions src/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,14 @@ jQuery.fn = jQuery.prototype = {
// Get the Nth element in the matched element set OR
// Get the whole matched element set as a clean array
get: function( num ) {
return num != null ?

// Return just the one element from the set
( num < 0 ? this[ num + this.length ] : this[ num ] ) :
// Return all the elements in a clean array
if ( num == null ) {
return slice.call( this );
}

// Return all the elements in a clean array
slice.call( this );
// Return just the one element from the set
return num < 0 ? this[ num + this.length ] : this[ num ];
},

// Take an array of elements and push it onto the stack
Expand Down
15 changes: 9 additions & 6 deletions src/core/access.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,16 @@ var access = function( elems, fn, key, value, chainable, emptyGet, raw ) {
}
}

return chainable ?
elems :
if ( chainable ) {
return elems;
}

// Gets
if ( bulk ) {
return fn.call( elems );
}

// Gets
bulk ?
fn.call( elems ) :
len ? fn( elems[ 0 ], key ) : emptyGet;
return len ? fn( elems[ 0 ], key ) : emptyGet;
};

return access;
Expand Down
16 changes: 9 additions & 7 deletions src/css.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,17 @@ function setPositiveNumber( elem, value, subtract ) {
}

function augmentWidthOrHeight( elem, name, extra, isBorderBox, styles ) {
var i = extra === ( isBorderBox ? "border" : "content" ) ?

// If we already have the right measurement, avoid augmentation
4 :
var i,
val = 0;

// Otherwise initialize for horizontal or vertical properties
name === "width" ? 1 : 0,
// If we already have the right measurement, avoid augmentation
if ( extra === ( isBorderBox ? "border" : "content" ) ) {
i = 4;

val = 0;
// Otherwise initialize for horizontal or vertical properties
} else {
i = name === "width" ? 1 : 0;
}

for ( ; i < 4; i += 2 ) {

Expand Down
34 changes: 26 additions & 8 deletions src/data.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,31 @@ define( [
var rbrace = /^(?:\{[\w\W]*\}|\[[\w\W]*\])$/,
rmultiDash = /[A-Z]/g;

function getData( data ) {
if ( data === "true" ) {
return true;
}

if ( data === "false" ) {
return false;
}

if ( data === "null" ) {
return null;
}

// Only convert to a number if it doesn't change the string
if ( data === +data + "" ) {
return +data;
}

if ( rbrace.test( data ) ) {
return JSON.parse( data );
}

return data;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


function dataAttr( elem, key, data ) {
var name;

Expand All @@ -31,14 +56,7 @@ function dataAttr( elem, key, data ) {

if ( typeof data === "string" ) {
try {
data = data === "true" ? true :
data === "false" ? false :
data === "null" ? null :

// Only convert to a number if it doesn't change the string
+data + "" === data ? +data :
rbrace.test( data ) ? JSON.parse( data ) :
data;
data = getData( data );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the size difference after extracting this to a function?

Copy link
Member Author

@markelog markelog Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull costs 32 bytes, not sure about this specific case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems every other change should be optimizable by UglifyJS so 32 bytes seems to be quite a lot...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it weird that I find the ternary more readable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgol switch..case usually not optimized by the engines or the minificators :/, we can try if..else though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider disabling no-nested-ternary... this is getting too aggressive for too little benefit (and even negative benefit, though I know which side of that argument I'm on).

} catch ( e ) {}

// Make sure we set the data so it isn't changed later
Expand Down
11 changes: 8 additions & 3 deletions src/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -453,9 +453,14 @@ jQuery.speed = function( speed, easing, fn ) {
opt.duration = 0;

} else {
opt.duration = typeof opt.duration === "number" ?
opt.duration : opt.duration in jQuery.fx.speeds ?
jQuery.fx.speeds[ opt.duration ] : jQuery.fx.speeds._default;
if ( typeof opt.duration !== "number" ) {
if ( opt.duration in jQuery.fx.speeds ) {
opt.duration = jQuery.fx.speeds[ opt.duration ];

} else {
opt.duration = jQuery.fx.speeds._default;
}
}
}

// Normalize opt.queue - true/undefined/null -> "fx"
Expand Down
14 changes: 13 additions & 1 deletion src/event.js
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,19 @@ jQuery.each( {

// Add which for click: 1 === left; 2 === middle; 3 === right
if ( !event.which && button !== undefined && rmouseEvent.test( event.type ) ) {
return ( button & 1 ? 1 : ( button & 2 ? 3 : ( button & 4 ? 2 : 0 ) ) );
if ( button & 1 ) {
return 1;
}

if ( button & 2 ) {
return 3;
}

if ( button & 4 ) {
return 2;
}

return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet, I find these if statements much more readable than that ternary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not everything could be generalized, but in general nested or in this case deep nested ternaries are less readable.

For me thought, this is much more clearer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if you have objections, it would be better to get them from the start - jquery/eslint-config-jquery#2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes it's hard to know if you like something or not until you see how it affects the code though. I like this rewritten code because it's easy to see the returns and know nothing below it matters for that case. With the ternaries it's much harder (for me at least) to figure that out, even when split across lines.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes it's hard to know if you like something or not until you see how it affects the code though

yeeeah, i guess i get that

Copy link
Member

@timmywil timmywil Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was looking into this further. I think this code was only for IE6-8. I noticed that this fallback expects quirksmode behavior (http://www.quirksmode.org/js/events_properties.html#button), not the standard behavior of event.button. I think we can remove this code!

In other words, if the reason for leaving this in was that which might get removed, this code wouldn't work anyway...http://jsbin.com/hureba/edit?html,js,console,output

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, awesome! Let's consider this in the next pr though, let's kill that baby :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timmywil I think we still need event.which mapping, see gh-2337, I think a lot of code still depends on it.

Copy link
Member

@timmywil timmywil Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmethvin I'm not sure you understood what I meant. Note that the if ( !event.which block is only for IE6-8 and would not work right in modern browsers. We can leave the mapping and still remove that block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, was about do open ticket about this find and then there is Dave - #3235 :)

}

return event.which;
Expand Down
26 changes: 17 additions & 9 deletions src/manipulation/getAll.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,23 @@ function getAll( context, tag ) {

// Support: IE <=9 - 11 only
// Use typeof to avoid zero-argument method invocation on host objects (#15151)
var ret = typeof context.getElementsByTagName !== "undefined" ?
context.getElementsByTagName( tag || "*" ) :
typeof context.querySelectorAll !== "undefined" ?
context.querySelectorAll( tag || "*" ) :
[];

return tag === undefined || tag && jQuery.nodeName( context, tag ) ?
jQuery.merge( [ context ], ret ) :
ret;
var ret;

if ( typeof context.getElementsByTagName !== "undefined" ) {
ret = context.getElementsByTagName( tag || "*" );

} else if ( typeof context.querySelectorAll !== "undefined" ) {
ret = context.querySelectorAll( tag || "*" );

} else {
ret = [];
}

if ( tag === undefined || tag && jQuery.nodeName( context, tag ) ) {
return jQuery.merge( [ context ], ret );
}

return ret;
}

return getAll;
Expand Down
18 changes: 11 additions & 7 deletions src/serialize.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,17 @@ jQuery.fn.extend( {
.map( function( i, elem ) {
var val = jQuery( this ).val();

return val == null ?
null :
jQuery.isArray( val ) ?
jQuery.map( val, function( val ) {
return { name: elem.name, value: val.replace( rCRLF, "\r\n" ) };
} ) :
{ name: elem.name, value: val.replace( rCRLF, "\r\n" ) };
if ( val == null ) {
return null;
}

if ( jQuery.isArray( val ) ) {
return jQuery.map( val, function( val ) {
return { name: elem.name, value: val.replace( rCRLF, "\r\n" ) };
} );
}

return { name: elem.name, value: val.replace( rCRLF, "\r\n" ) };
} ).get();
}
} );
Expand Down
12 changes: 7 additions & 5 deletions src/traversing/findFilter.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,13 @@ jQuery.filter = function( expr, elems, not ) {
expr = ":not(" + expr + ")";
}

return elems.length === 1 && elem.nodeType === 1 ?
jQuery.find.matchesSelector( elem, expr ) ? [ elem ] : [] :
jQuery.find.matches( expr, jQuery.grep( elems, function( elem ) {
return elem.nodeType === 1;
} ) );
if ( elems.length === 1 && elem.nodeType === 1 ) {
return jQuery.find.matchesSelector( elem, expr ) ? [ elem ] : [];
}

return jQuery.find.matches( expr, jQuery.grep( elems, function( elem ) {
return elem.nodeType === 1;
} ) );
};

jQuery.fn.extend( {
Expand Down
1 change: 0 additions & 1 deletion test/.eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
"brace-style": "off",
"key-spacing": "off",
"camelcase": "off",
"dot-notaion": "off",

// Not really too much - waiting autofix features for these rules
"lines-around-comment": "off",
Expand Down