[lab] Migrate Timeline to TypeScript #23242
Conversation
github-actions
bot
added
the
PR: out-of-date
oliviertassinari
force-pushed
the
lab-host-typescript
branch
3 times, most recently
from
372dd52
to
b312efc
Compare
github-actions
bot
removed
the
PR: out-of-date
oliviertassinari
force-pushed
the
lab-host-typescript
branch
2 times, most recently
from
0340f74
to
a3f2f79
Compare
oliviertassinari
added
core
oliviertassinari
force-pushed
the
lab-host-typescript
branch
from
a3f2f79
to
386de4d
Compare
oliviertassinari
force-pushed
the
lab-host-typescript
branch
from
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.
oliviertassinari
force-pushed
the
lab-host-typescript
branch
from
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