converts ComboBox family of components to TypeScript

[dimitropoulos]

closes: #2664

This is a draft pull request (i.e. work in progress) on converting the ComboBox family to TypeScript.

There are quite a few tough cookies to crack here, haha, I can see why no one has done it yet. Still, this is a huge step in the right direction.

I'm planning on taking quite explicit notes on all notable changes, but I wanted to throw this PR up just to give a first look at where it's headed.

[kibanamachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[dimitropoulos]

Please suggest a name, haha, all the good ones seem to be taken. I left it as OptionOption just so that I could keep moving, unblocked by the need for a name.

[thompsongl]

I'll think on it. OptionOption is fine for the time being.

[dimitropoulos]

@chandlerprall @thompsongl I sat down with this again. Made some good progress, but I still have something of a sticky-web to keep unweaving with refs. I'm definitely not ready for a full review yet (I have some NOTES_TO_SELF laying around that I'll clean up before marking the PR Ready for review and a couple other little things I know are broken but haven't finished investigating yet), but I would like you to take a look at just one thing:

I haven't done an extensive git-blame but it appears that, in the context of being a prop, singleSelection for both EuiComboBox and EuiComboBoxInput:

export interface EuiComboBoxSingleSelectionShape {
  asPlainText?: boolean;
}

Was added after the singleSelection prop was just a simple boolean. There are places in the code that assume that it's one or the other, for example:

here's one that seems to assume it'll be an object, always.

and here's one that seems to assume it'll be a boolean, always.

I tried to massage things into place, but I think I'm uncomfortable with the "level" of changes that I'd have to do in the context of a PR like this, which, in my mind, should make as few logical changes as possible.

Your input is appreciated.

[thompsongl]

@dimitropoulos Thanks for the status update and for helping move this along!

singleSelection is certainly a unique case 😄

The EuiComboBoxProps interface had it specified correctly:

singleSelection?: EuiComboBoxSingleSelectionShape | boolean;

The "Single selection" section it the docs explains it best, I think:

To only allow the user to select a single option, provide the singleSelection prop. You may want to render the selected option as plain text instead of pill form. To do this, pass singleSelection={{ asPlainText: true }}

So passing an object containing asPlainText essentially sets singleSelection to true and provides an extra bit of rendering logic. Not sure, but it was probably done so to prevent adding Yet Another top-level boolean prop.

The logic in both checks you mention appear to accomplish what they mean to given the truthy-falsy nature of the prop.

[dimitropoulos]

ok. cool. I fixed in f805215. Yeah, I've learned not to make many assumptions since usually at least one of the below surprises someone that touched the code (especially where types were/are not present):

console.log(classnames({
  a: true, // adds classname
  b: false,
  c: 1, // adds classname
  d: 0,
  e: -1, // adds classname
  f: 'true', // adds classname
  g: 'false', // adds classname
  h: '1', // adds classname
  i: '0', // adds classname
  j: '-1', // adds classname
  k: '',
  l: null,
  m: undefined,
  n: Infinity, // adds classname
  o: -Infinity, // adds classname
  p: [], // adds classname
  q: {}, // adds classname
  r: [[]], // adds classname
  s: [0], // adds classname
  t: [1], // adds classname
  u: NaN,
  v: () => {},
})); // => a c e f g h i j n o p q r s t v

(and more)

---

Also, consider the following code:

eui/src/components/combo_box/combo_box_input/combo_box_input.js

Line 119 in f11ea60

const asPlainText = singleSelection && singleSelection.asPlainText;

If singleSelection === true, then asPlainText === undefined. I get that this is falsey so it's kinda whatever but I just wanted to highlight that case. The same would occur if singleSelection === {}.

---

Lastly, in my commit fixing this I adhered to https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md. This means there are a two or three Boolean() usages that aren't strictly necessary for the code to function. I'm more than happy to remove those extra usages if you would prefer. I feel it adds to the clarity of the code for a near-zero runtime cost, but, again, I'm more than happy to go with whatever :)

[thompsongl]

This means there are a two or three Boolean() usages that aren't strictly necessary

We'd be ok with Boolean use. The places you've done so make sense.

[thompsongl]

Mostly some missing CommonProps extensions

[thompsongl]

jenkins test this

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_2838/

[thompsongl]

This looks ready to go.

Thanks for your work and patience, @dimitropoulos!

[snide]

Thanks @dimitropoulos for this PR. Certainly a big one and it's a big help getting the library closer to being fully TS!

[dimitropoulos]

thanks all!

and as with all the others: if any kind of "this was prop was typed as not optional but actually it's supposed to be optional" thing comes up with this or any of the others - please feel free to ping me. I can only work on this project nights/weekends but I'll do my best to help if I made any mistakes. :)