[EuiSelectable] `data` prop in `EuiSelectableOption` can never be typed correctly by consumers

[pickypg]

[EUI team note: the original filed issue had a valid workaround, but in the process we discovered a Typescript issue that could use resolution.]

Problem

Typescript issue: The renderOptions prop does not correctly type the option being sent back to the consumer, due to us enforcing data: { [key: string]: any } as well as munging the data obj as top-level keys.

In the TSX demo linked above, the typing stops working once the optional notation is removed from secondaryContent?: https://codesandbox.io/s/adoring-allen-oxeh2u?file=/demo.tsx:1420-1448

Solutions

See the GitHub comment chian starting at https://github.com/elastic/eui/issues/6803#issuecomment-1560196819 for more information and potential fixes discussed.

Click to expand original issue

**Describe the bug** `EuiSelectable` does not support internationalization (I18n) without triggering warnings every time that it is rendered. When using things like I18n, you cannot depend on the `label` being your match-value, hence the need for an optional `value?: string`. **Environment and versions** - EUI version: Latest - Browser: Chrome - Operating System: macOS **To Reproduce** Steps to reproduce the behavior: 1. Create `EuiSelectable` 2. Provide options that use / require I18n 3. See warnings **Expected behavior** A clear and concise description of what you expected to happen. **Minimum reproducible sandbox** https://codesandbox.io/s/sweet-ives-md3deg?file=/demo.js **Screenshots** **Additional context** `EuiSuperSelect` is basically the desired output, except that it cannot be used in the same manner via a prepended form control because it does not blend in the same way as an `EuiButtonEmpty` does with an `EuiPopover` next to a search box _and_ it only supports single select (which is actually perfectly fine for my use case, but obviously not all use cases).

[cee-chen]

searchableLabel already exists as a corollary to value:

https://github.com/elastic/eui/blob/f8bc384be01dbf755383e8933909f8872e1cab36/src/components/selectable/selectable_option.tsx#L20-L24

While we could certainly change label to accept any ReactNode, that would most likely be flagged as a breaking change as we'd need to check for typeof label === 'string' for and potentially throw an error if a string label and searchableLabel doesn't exist.

I wonder if a potentially less breaking change would be to introduce a new optional display property for each option - not sure if feels too confusing when used with renderOption at the same time.

[pickypg]

Thanks for highlighting searchableLabel, although that impacts search for those that use it (I'm not, so I can make use of it).

I wonder if a potentially less breaking change would be to introduce a new optional display property for each option - not sure if feels too confusing when used with renderOption at the same time.

This is fine for me where it could be an exclusive one-or-the-other thing.

[cee-chen]

Gotcha, EuiSelectable is used so often for search/filtering options I totally hyperfocused on that. I believe the ability to parse the label as text for search/highlight functionality is one of the main reasons it was typed as a string originally - other than that, it could be a React Node for all we care.

Just curious, since you opened this issue around i18n specifically - is there a reason why you wouldn't want to use either EUI or Kibana i18n utils that return a string? For EUI, this would be useEui18n, for Kibana, it would be i18n.translate() instead of <FormattedMessage>.

[pickypg]

I only have the EUI variant accessible to my code, but the list is dynamic and does not support the use of the hook-variant of it as a result. I had actually jumped through a bunch of hoops to make it workable with that, but it ultimately triggered a lot of extra re-renders just to use it.

[cee-chen]

Gotcha, thanks for that explanation.

After taking a closer look at the errors around <EuiHighlight>, I'm a little more hesitant to make this change (even just adding a display prop), just because searching and highlighting the search is such a large part of EuiSelectable behavior and what a majority of our consumers (as well as internal EUI dogfooding of the component) use it for.

I think your specific usage of EuiSelectable might be a bit of a usage edge case, and as such I'd recommend a workaround that uses renderOption and custom data to display your i18n components:

https://codesandbox.io/s/gifted-wu-zxf05m?file=/demo.js:607-655

[pickypg]

I somehow didn't see renderOption. This likely solves my problem completely.

[cee-chen]

No problem at all!! Just glad EUI has an existing escape hatch (of sorts) in place for your use case. Thanks for talking through the problem with us as always! ❤️

[pickypg]

This does indeed fix my issue. The only thing I would note is that the behavior, in Typescript, of moving the data properties to the option in renderOption is odd and it requires a cast that shouldn't be required.

-  renderOption={(option) => <>{option.display}</>}
+  renderOption={(option) => <>{option.data.display}</>}

As it is (the - portion above), it requires a cast because the data was hoisted, but I don't see the advantage of doing it versus just leaving it in data and allowing the developer to use it from there, which the developer could even provide a type for EuiSelectable (e.g., EuiSelectable<CategoryOption>, which it already supports) to take advantage of.

[cee-chen]

👍 Yea it looks like data is just straight up getting merged in with other options:

https://github.com/elastic/eui/blob/329eb41a8c36ba76be780fc6a45d375cf7184033/src/components/selectable/selectable_list/selectable_list.tsx#L321-L322

It's definitely a bit of an odd choice 😅 We probably should have gotten more consumer feedback on that before we implemented it that way. I'm a bit 50/50 on changing it now (since it creates a breaking change), but I definitely see your point that it makes typing annoying.

[pickypg]

Perhaps a nice middle ground would be not explicitly removing data from option so that it can coexist assuming the user doesn't overwrite it (meaning they did data: { data }).

[pickypg]

As a now-consumer of it, it's definitely a wanted breaking change because as it stands I have a 3-line comment about my as any cast.

[cee-chen]

Let's just quickly check the impact of the downstream breaking change - it looks like there's a fairly high amount of renderOption usages in Kibana, although some of those are EuiComboBox and not of EuiSelectable.

It looks like you should be able to use a TS generic to get this working without an as cast via EuiSelectableOption<SomeDataInterface>:

https://github.com/elastic/eui/blob/e95d31b245e73ca0cf943640a1b722d4012812df/src-docs/src/views/selectable/selectable_custom_render.tsx#L65-L81

It also looks like you could just pass in that typing directly to EuiSelectable<SomeDataInterface>, unless I'm wrong:

https://github.com/elastic/eui/blob/f288e12d5e4ad0b12e3d98d8877fcc1397f50478/src/components/selectable/selectable.tsx#L189

Let me know if that works for you?

[pickypg]

It also looks like you could just pass in that typing directly to EuiSelectable, unless I'm wrong: Let me know if that works for you?

The example is cheating by specifying the data portion as a direct part of the EuiSelectableOption as it is passed to renderOption. Technically, I could do the same thing by making another interface / type, but what's happening here is a trap for some future developer. For reference, I am already using the generic for it like:

type CategoryOption = EuiSelectableOption & {
  data: { display: JSX.Element };
  searchableLabel: string;
};

and then loading the EuiSelectable with it as:

<EuiSelectable<CategoryOption>
  aria-label="Select a category"
  // ...
  renderOption={(option) => (option as any).display}
/>

Of interest, Typescript thinks that option.data.display exists because the typing is broken by renderOption, even though it does not (if I changed it to renderOption={(option) => option.data.display}, it triggers a crash even thought Typescript is happy).

[cee-chen]

Here's the issue with your usage, I think:

- type CategoryOption = EuiSelectableOption & {
-   data: { display: JSX.Element };
-   searchableLabel: string;
- };
+ type CategoryOptionData = { display: JSX.Element }; // And whatever other custom keys you have
+ type CategoryOption = EuiSelectableOption<CategoryOptionData>

You're extending EuiSelectableOption instead of passing it a generic. Similarly, with EuiSelectable:

+ type CategoryOptionData = { display: JSX.Element };
- <EuiSelectable<CategoryOption
+ <EuiSelectable<CategoryOptionData

[cee-chen]

Oh wait no, you're totally right. The reason why that example works is because secondaryContent?: is optional - I start getting errors if I remove that ?. Let me re-investigate our typing here really quick 😬 Ideally this should just work as I wrote above.

[pickypg]

I think the issue is that T bumps with the data?: { [key: string]: any }; and we're both expecting T to become data given how it messes with data. However, because we have T and the data hoisting, we can simply bypass the need to use data at all by supplying our parameters separately from data.

For example:

type CategoryOption = {
  display: JSX.Element;
  value: string;
};

const CATEGORIES: EuiSelectableOption<CategoryOption>[] = SEARCH_CATEGORIES.map(
  ({ key, value }) => ({
    display: <EuiI18n token={`${key}.plural`} default={value} />,
    label: key,
    value,
  })
);

<EuiSelectable<CategoryOption>
  aria-label="Select a category"
  // ...
  renderOption={(option) => option.display}
/>

[cee-chen]

What you wrote works perfectly with TS, but React will yell about it because we extract the ...rest of options and pass it down to the underlying <li>, which means you're getting a display attribute on your option DOM:

https://github.com/elastic/eui/blob/329eb41a8c36ba76be780fc6a45d375cf7184033/src/components/selectable/selectable_list/selectable_list.tsx#L317

Looking at the prop docs, that appears to be why Greg added the data property in the first place 😬

my 2c is that the 'cleanest' solution at this point would probably be to either:

1. Monkey around with Typescript more so that we correctly type the data we give to you in renderOptions vs the data you give to us in options
2. Breaking change: leave the data attribute intact instead of merging its children in as top level properties.

I'll poke at 1 a bit further as breaking changes can be a bit of a downstream headache in Kibana.

[cee-chen]

I tried extending EuiSelectable's generics with a 2nd type for custom data, e.g. <EuiSelectable<T = {}, D = {}> and Typescript absolutely lost its mind 😆

After some thought, I'm still not 100% sure that option 2 will "just work", however. Unfortunately we've typed [data] pretty loosely ({ [key: string]: any }) with no meaningful way for consumers to pass in their custom data interface/type to EuiSelectable, so we can't force Typescript to infer the returned data no matter what.

[cee-chen]

So, I still don't have a good solution for the data type and passing in a specific generic/type for it (might have to punt that to the backlog for now), but I did come up with an alternative option for your i18n use case that doesn't require data if you're interested!

• It doesn't pass a JSX element via data - it makes more sense to do that directly renderOption instead 😅
• It bogarts the key "prop", although if this feels hacky to you or if you're reusing token names, you could also do something like data-i18n-key/data-i18n-token (if you're fine with that token showing up in the DOM)

https://codesandbox.io/s/wonderful-dust-uwk7ki?file=/demo.tsx

[pickypg]

I think that, for my usage, I can safely just supply the key as the label and then use my custom value field to render the EuiI18n dynamically.

<EuiSelectable<CategoryOption>
  aria-label="Select a category"
  // ...
  renderOption={({ label, value }) => (
    <EuiI18n token={`${label}.plural`} default={value} />
  )}
/>

I now wonder if a second parameter could be added to the renderOption that is passed the data object, which then could be munged together with the T, which would allow existing code to work as-is and new usages to intentionally reinterpret data by updating T to expose it. This would helpfully avoid the annoying DOM issue while providing for the intent here.

[github-actions[bot]]

👋 Thank you for your suggestion or request! While the EUI team agrees that it's valid, it's unlikely that we will prioritize this issue on our roadmap. We'll leave the issue open if you or anyone else in the community wants to implement it by contributing to EUI. If not, this issue will auto close in one year.

Log | Bot Usage

[cee-chen]

As a heads up, I've gone ahead and tweaked the original PR title and description to describe the work that needs to be done (primarily around Typescript) as well as applied the correct labels. Ignore the bot saying this issue will auto-close, bugs will not auto-close (which is what I would categorize this as).

As a further heads up, this is likely pretty low priority for EUI, so we have no ETA for a fix, but if you (or anyone out there) has time or interest we'd absolutely accept a community PR/contribution on this!

[zinckiwi]

A +1 on "would like to see this". I went to const options = Array<EuiSelectableOption<ProjectType>> and was surprised to find it doesn't work.

There's also this weird issue once you add a generic param:

type ProjectType = "elasticsearch" | "observability" | "security"

const PROJECT_TYPES: ProjectType[] = ['elasticsearch', 'observability', 'security']

const options: EuiSelectableOption<ProjectType>[] = PROJECT_TYPES.map((type) => ({
  key: type,
  label: type,
  checked: solution === type ? 'on' : undefined,
}))

// Property 'isGroupLabel' is missing in type
// '{ key: ProjectType; label: ProjectType; checked: "on" | undefined; }'
// but required in type '{ isGroupLabel: true; }'.

[cee-chen]

I've run into the isGroupLabel type error shenanigans as well, and that's definitely EUI being overly aggressive about using our custom ExclusiveUnion utility (which can be pretty hit or miss, just IME).

My vote would probably be to nuke the ExclusiveUnion usage when we fix this bug, and use an OR operator instead, e.g. EuiSelectableGroupLabelOption<T> | EuiSelectableLIOption<T>.