Selector: Properly implement :enabled/:disabled for select contents

[gibson042]

SemVer patch
Fixes gh-382
Ref jquery/jquery#3224
Ref 7999a01

[markelog]

Looks like you really don't like jQuery eslint config :/

[gibson042]

5dab06f already converted a ternary expression to logical statements; let me see what the hit is from introducing one or two more levels of nested ifs. But yes, code like this is an example of why I don't like no-nested-ternary—every fork of this complex decision tree¹ has the single purpose of returning a boolean value indicating either enabledness or disabledness depending on outer function argument, and IMO that structure isn't rendered clearer by replacing expressions with statements. Since it's always going to be hairy, I'd rather optimize for size and handle comprehensibility with blank lines, indentation, and comments.

¹ in statement-based pseudocode:

if likely a listed form element:
  if disabledness might be inherited:
    if an option:
      if parent is an optgroup:
        parent.disabled represents disabledness
      else:
        elem.disabled represents disabledness
    else if elem.isDisabled is boolean:
      elem.isDisabled represents disabledness
    else:
      the existence of a disabled fieldset ancestor represents disabledness
  else:
    elem.disabled represents disabledness
else if likely an optgroup or menuitem:
  elem.disabled represents disabledness
else:
  neither enabled nor disabled

[markelog]

That's how parser code usually looks like (with if..else not with ternaries, which is easy to understand to me at least), essentially this part is a parser but working with different data type - not strings but DOM, never saw a parser that love ternaries like that, in many modern languages, for example like go or rust ternary do not exist.

And from my experience, when you write code with simple if..else you usually find ways to move out some parts of scope to helper functions which decreases cyclomatic complexity or easily find a ways to simplify and optimize things but never with ternaries.

I'd rather optimize for size

That's unclear, there is no evidence to it, from writing code for core with replacing such constructions, I can say that uglify was always outputed same gzip says as with ternaries.

Because you writing code like this and still using jshint and jscs it prevents us from enabling correct full check for dist version of jQuery - https://github.com/jquery/jquery/blob/master/dist/.eslintrc#L8-L17, currently, there is 10 rules we had to disable because of it.

In any case if you had objections to it, i think you had to bring it up beforehand - jquery/eslint-config-jquery#2 I cc everyone to that ticket

[gibson042]

That's how parser code usually looks like (with if..else not with ternaries, which is easy to understand to me at least), essentially this part is a parser but working with different data type - not strings but DOM, never saw a parser that love ternaries like that, in many modern languages, for example like go or rust ternary do not exist.

Go has named return values, and Rust has if-expressions. This is Javascript, where we have neither. Languages are different.

And from my experience, when you write code with simple if..else you usually find ways to move out some parts of scope to helper functions which decreases cyclomatic complexity or easily find a ways to simplify and optimize things but never with ternaries.

I had already done that with the ternaries, which as I pointed out are exactly equivalent to the statements, since the result is always a single equality comparison expression. Which brings me to...

I'd rather optimize for size

That's unclear, there is no evidence to it, from writing code for core with replacing such constructions, I can say that uglify was always outputed same gzip says as with ternaries.

As it turns out, this function is now in a form where Uglify can produce the exact nested ternaries that I want, so I've implemented the statement-based version.

Because you writing code like this and still using jshint and jscs it prevents us from enabling correct full check for dist version of jQuery - https://github.com/jquery/jquery/blob/master/dist/.eslintrc#L8-L17, currently, there is 10 rules we had to disable because of it.

I'm blocked on handling of expression parentheses (eslint/eslint#4689 (comment) , eslint/eslint#5270 ). Sizzle has lots of assignments inside statement conditions, and we use tight parentheses to indicate their intentionality.

In any case if you had objections to it, i think you had to bring it up beforehand - jquery/eslint-config-jquery#2 I cc everyone to that ticket

Like @dmethvin said, the consequences weren't obvious. I for one am frustrated by the separation of variable declarations from initializations that was required in jQuery core to satisfy no-nested-ternary.

[markelog]

Go has named return values

Well, return and ternary not always go together

Rust has if-expressions

Relevant - rust-lang/rust#1698 (comment)

As it turns out, this function is now in a form where Uglify can produce the exact nested ternaries that I want, so I've implemented the statement-based version.

I'm blocked on handling of expression parentheses

We can disable parentheses rules at this point I think (keep in mind that eslint has a lot of issues so team "buried" by them, so there is a good chance those are not gonna be resolved in the near future) important part is to start linting maybe not fully, but at least partially. Like doing the work you started in #330

Like @dmethvin said, the consequences weren't obvious.

If ticket is closed it doesn't mean we can't restart discussion in there or we can re-open it

[timmywil]

Just curious, does the else here help uglify produce the desired ternary?

[gibson042]

It's not necessary, I just used it to establish the especially close link between those branches (i.e., option-in-optgroup vs. naked option).

[markelog]

Yeah, had the same question, I think it is established practise to remove redundant else statements, but it is not contradicts our style guide