[lab] Migrate Timeline to TypeScript#23242
Conversation
372dd52 to
b312efc
Compare
0340f74 to
a3f2f79
Compare
a3f2f79 to
386de4d
Compare
386de4d to
6f0972f
Compare
| /** Styles applied to the root element if `align="alternate"`. */ | ||
| alignAlternate?: string; | ||
| }; | ||
| ref?: React.Ref<HTMLUListElement>; |
There was a problem hiding this comment.
I couldn't use:
| ref?: React.Ref<HTMLUListElement>; | |
| ref?: React.Ref<unknow>; |
which is the default
| ref?: React.Ref<HTMLUListElement>; |
it fails with a "can't assign unknown to HTMLUListElement" error message if I don't set the correct type.
There was a problem hiding this comment.
The outer interface is more important here. I've gone through length to find the best possible type for ref so we shouldn't revert this partially just because the inner types are annoying. This is part of why I have problems with migrating to typescript: it might regress the DX for library users.
There was a problem hiding this comment.
microsoft/TypeScript#30748 and #15199 for more context
There was a problem hiding this comment.
Ok, so we can simply:
// @ts-expected-error TypeScript bug, need to keep unknown for DX?
I like that :) (cc @dtassone ;))
There was a problem hiding this comment.
I wouldn't say you can do anything "simply" in TypeScripts' JSX domain 😀 It's just bit unfortunate that this pattern of assigning and reading refs isn't supported by TS because we don't have control over covariance vs contravariance in function calls. But yes, you probably can @ts-expect-error assignments of less specific refs to the ref prop.
6f0972f to
286df26
Compare
| <TimelineContext.Provider value={{ align }}> | ||
| <ul | ||
| className={clsx( | ||
| classes!.root, |
There was a problem hiding this comment.
I'm not super happy about this one, but it seems to be a reasonable tradeoff.
There was a problem hiding this comment.
You're only typing the outer props i.e. the props a user can pass in but these are not the same props the component actually receives due to higher-order components.
We need to come up with a robust approach once we're starting to switch to emotion so that we don't degrade user types. Not important for this PR but v5 release.
I have extracted some of the changes from #22692 and completed the missing part to allow migrating the
Timelinecomponent to TypeScript. I have used the Timeline as it's simple enough to test the direction. As far as I know, this would make the very first component written in TypeScript.Related to #15984