-
Notifications
You must be signed in to change notification settings - Fork 563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Types: Add types to <EditableList /> component #1886
Conversation
// Language and Platform | ||
/////////////////////////////////////// | ||
|
||
export type Without<T, U> = { [P in Exclude<keyof T, keyof U>]?: never }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like these need some explanation as to what they are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can do and will add. I feel like XOR
should be in the language. it's here because the |
is only really useful when we have a property on which to discriminate alternative types.
type Colors = { name: 'red', value: 0xff000 } | { name: 'green', value: 0x00ff00 };
This fails if our types our different
type Size = { radius: number } | { width: number; height: number };
It should be able to do this but it can't. When we don't have that property we have to rely on more advanced typing. In this case it's a mapped type called XOR
which does exactly what |
seems like it should do. Without
is part of it and gets the difference of keys between them. Here I've used XOR
because MouseEvent
and TouchEvent
don't have the discriminating property.
type Size = XOR<{radius: number}, {width: number; height: number}>
We can keep adding to the types by nesting, unlike the convenient chaining of |
type Size = XOR<{radius: number}, XOR<{width: number; height: number}, {kg: number}>>
lib/editable-list/index.tsx
Outdated
static propTypes = { | ||
className: PropTypes.string, | ||
editing: PropTypes.bool.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why leave this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh. missed that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it!
As we changed some code here, specifically around initializing instance variables, we need to be extra careful in testing to prevent regressions.
19190e3
to
ad6a836
Compare
Testing in batch with other PRs merging today. |
As we changed some code here, specifically around initializing instance
variables, we need to be extra careful in testing to prevent regressions.