-
-
Save cchaos/56d0bcfe0c4f4fb47148056bf1627b77 to your computer and use it in GitHub Desktop.
| import React from 'react'; | |
| import { ExclusiveUnion } from '../common'; | |
| /** | |
| * 1. Use the EUI provided ExclusiveUnion to separate unique props. | |
| */ | |
| type _RestrictedProps = ExclusiveUnion< | |
| { | |
| /** | |
| * 2. At least one of the unique props must be required, usually the one containing more options | |
| */ | |
| position: 'fixed'; | |
| /** | |
| * 3. These next props are only available when `position` is `fixed` | |
| */ | |
| foo?: boolean; | |
| bar?: boolean; | |
| }, | |
| { | |
| /** | |
| * 4. These are the other options that can't be used in conjunction with the above props | |
| */ | |
| position?: 'static' | 'sticky'; | |
| } | |
| >; | |
| export interface EuiComponentProps | |
| extends _RestrictedProps { | |
| /** | |
| * 5. Add all your other props that are allowed no matter the other configurations | |
| */ | |
| stuff?: string; | |
| } | |
| export const EuiComponent: FunctionComponent<EuiComponentProps> = ({ | |
| position = 'fixed', | |
| foo = true, | |
| bar = true, | |
| stuff, | |
| ...rest | |
| }) => { | |
| /** | |
| * 6. Still ensure the props values are correct for the particular configuration | |
| */ | |
| foo = position !== 'fixed' ? false : foo; | |
| bar = position !== 'fixed' ? false : bar; | |
| return (); | |
| } |
| import React from 'react'; | |
| import { ExclusiveUnion } from '@elastic/eui'; | |
| export interface MyComponentProps { | |
| /** | |
| * 7. This component allows consumers to override defaults supplied to the resctricted component | |
| */ | |
| overrides?: EuiComponentProps; | |
| } | |
| export const MyComponent: FunctionComponent<MyComponentProps> = ({ | |
| overrides, | |
| }) => { | |
| return ( | |
| <EuiComponent | |
| position="sticky" | |
| // 8. Typescript will always complain, so using unknown here, | |
| // but it won't restrict consumers from providing incompatible props, so ensure the EUI component handles that (see #6) | |
| {...(overrides as unknown)} | |
| /> | |
| ); | |
| } |
Honestly, I don't know when it's supposed to be type vs interface. From other docs I've ready, people prefer interface, but I don't really know the difference. Also, I've truncated that quite a bit. There's a lot more props in there, but I just wanted to simplify for the gist.
interfaces are more restricted, because they can only express object types. AFAIK the only thing possible with interfaces that plain object types can't do is declaration merging. In my experience this is mostly used to deal with the dynamic nature of javascript code that was not written with strong typing in mind.
Converting EuiBottomBarProps to a type is fine; the ExclusiveUnion usage means that TS cannot, without more information, know the exact shape of the interface because it depends on if the input contains position: fixed, which is what TS is somewhat cryptically referring to with
TS2312: An interface can only extend an object type or intersection of object types with statically known members.
So we convert to (dropping point #5 as well because its redundancy is not needed),
export type EuiBottomBarProps = CommonProps &
HTMLAttributes<HTMLElement> &
_BottomBarExclusivePositions;Which looks like it resolves the rest of the issues you encountered. My goto rule with choosing interface vs. type: start with interface but upgrade to type when you have to. The exact differences are difficult to follow and often change subtly between TS releases.
My goto rule with choosing interface vs. type: start with interface but upgrade to type when you have to.
Same.
I think it's 👍 for these to be intersection types.
Thanks! I've converted it all to use type. And it's working well, except for one use case. Where I'm using this component inside of another component with a few default props setup but the user can override them. The way we normally handle this, is just by applying the props we want then spreading the overridable props at the end. Like so:
<EuiBottomBar
paddingSize={paddingSize}
position="sticky"
{...bottomBarProps}
/>
But this particular setup blurps an error:
TS isn't able to understand the ExclusiveUnion through another component's rest handling, unfortunately. We cast those spreads as the props, so: {...(bottomBarProps as EuiBottomBarProps)} (example at https://github.com/elastic/eui/blob/master/src/components/header/header_links/header_link.tsx#L52)
😕 It still likes to complain. Just differently...
Try moving position="sticky" below the bottomBarProps spread
Unfortunately, no, it still doesn't work and also removes the ability for users to change that prop.
In actuality, in this particular situation, I could omit those props, because they probably shouldn't be changing it, but it would be nice to have a workable solution for future components.
At this location, https://github.com/cchaos/eui/blob/feat/bottom_bar_in_page/src/components/bottom_bar/bottom_bar.tsx#L52, the following change is needed: position?: 'fixed'; -> position: 'fixed'; (removed the ?)
Reason: When TS validates an incoming value against the two sides of a union, it looks for unique key definitions. As it is right now, the two sides for position are undefined | 'fixed' and undefined | 'sticky' | 'static', and because both share the undefined value it is not unique. This prevents the expected validation, and also confused TS when it tried to understand the shape of bottomBarProps.
Which opens up a second issue, where the Partial in bottomBarProps?: Partial<EuiBottomBarProps>; again removes that distinction by making adding undefined back to position: 'sticky'. In this specific case the fix is simple: bottomBarProps?: EuiBottomBarProps;. This works because every prop in EuiBottomBarProps is already optional - except for position when opting into usePortal or affordForDisplacement. We'll need to explore other approaches for the case where a component provides default or un-overridable values.
Haha, apparently this one is being super tricky. Everything that you just said works and makes sense. Except now when I want to JUST adjust the prop allowed when position="fixed". So this:
<EuiBottomBar usePortal={false} />
But, even though position="fixed" is the default, TS complains telling me I need to add it.
❤️ Thank you all for your help and input. I've updated this gist to exemplify both the "finalized" component and usage that we can link to in our EUI wiki.
Maybe I'm missing some context, but is there any reason why
EuiBottomBarPropsneeds to be aninterface? Would an intersection type likebe an option too?