add field predicate types to selectionTest in vega-selections

[jonathanzong]

This change introduces the ability for vlSelectionTest to check for more types of field predicates.

This additive change will have no immediate effect, since Vega-Lite doesn't currently produce selection tuples that use these predicate types. However, future proposed changes for Animated Vega-Lite will require us to support these field predicates in addition to the existing point and interval predicates.

cc @arvind

[jonathanzong]

i believe this supersedes #1839 which is also very stale anyway

[domoritz]

Should this become a switch now?

[jonathanzong]

i personally don't find switch to be clearer in this instance because all of the returns are conditional, so we'd have to add break statements to every one and remember to keep adding them if we change it in the future. IMO, not shorter, possibly less maintainable, not a significant change in readability. however, happy to make the change if you feel otherwise

[domoritz]

Makes sense. I don't have a strong opinion. You could pull the logic here out into a function and then use a return in each branch.

[jonathanzong]

the if statements only return false because it's an early exit from the loop (none of the comparisons can be false for the test to return true), so we can't use an unconditional return inside the switch without changing the loop too

presumably it is written this way for efficiency(tm) over readability? but it did take me trying to rewrite it to realize this so maybe it should just be made more readable?

what do you think of e.g.

fields.every(f => {
  switch (f.type) {
    case TYPE_ENUM:
      return ...;
    case ...
  }
})

[arvind]

Your intuition is correct @jonathanzong! Because this code path is typically called against every visualized data point on every interaction (eg potentially every mouse drag), we need it to be really performant. When this was first written, most browsers were still faster at evaluating an old school for loop. It was also one of the reasons we minimized functions calls (because they can—or at least could—incur performance overhead).

So, while I’m sympathetic to readability concerns, if we really wanted to make a change here (including extracting this to another function call), perhaps we could run a micro benchmark to validate whether it would cause a performance hit?

(My guess is browsers have advanced significantly since these code paths were first written, so I wouldn’t be surprised if some of these reasons no longer hold. On the other hand, back in the day, I do remember being surprised by what syntaxes did and didn’t incur performance penalties!)

[jonathanzong]

bump @arvind

[domoritz]

Looks good to me but I will defer to @arvind for the final review.

[domoritz]

@arvind can you wrap up the review and merge if it looks good?