Build: Update eslint config and fix associated errors #3233
Conversation
| +data + "" === data ? +data : | ||
| rbrace.test( data ) ? JSON.parse( data ) : | ||
| data; | ||
| data = getData( data ); |
There was a problem hiding this comment.
What's the size difference after extracting this to a function?
There was a problem hiding this comment.
This pull costs 32 bytes, not sure about this specific case
There was a problem hiding this comment.
It seems every other change should be optimizable by UglifyJS so 32 bytes seems to be quite a lot...
There was a problem hiding this comment.
Is it weird that I find the ternary more readable?
There was a problem hiding this comment.
@mgol switch..case usually not optimized by the engines or the minificators :/, we can try if..else though
There was a problem hiding this comment.
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).
|
Related to ESLint configs: the |
Will fix that |
Wait, what do you mean? This rule used in |
|
ooh, i get it :) |
| return 2; | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
Yet, I find these if statements much more readable than that ternary.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Also, if you have objections, it would be better to get them from the start - jquery/eslint-config-jquery#2
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Wow, awesome! Let's consider this in the next pr though, let's kill that baby :)
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Heh, was about do open ticket about this find and then there is Dave - #3235 :)
|
I am in favor of every change but the data one. The switch statement is overly expansive. |
|
You guys are totally right, it saved 14 bytes, nice |
|
It'd be interesting to see what causes the +18 bytes increase (maybe sth could be optimized better by us or Uglify). But increased readability is worth 14 bytes anyway. |
| } | ||
|
|
||
| return data; | ||
| } |
|
It seems there is a consensus about this pr, as you probably know such prs get stale pretty quickly since they related to code style so the more prs land the harder to support it, so i would like to land it in couple hours |
|
No objections here. |
7 participants
Summary
Checklist
Mark an
[x]for completed items, if you're not sure leave them unchecked and we can assist.Thanks! Bots and humans will be around shortly to check it out.