[core] Type ref for components #15199
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eps1lon
added
new feature
|
No bundle size changes comparing 8db4931...533e32a |
This was technically incorrect since the override component didn't receive the props used in the parents but only excess props
eps1lon
force-pushed
the
feat/core/ref-types
branch
from
d46755a
to
21572a8
Compare
eps1lon
commented
Apr 4, 2019
packages/material-ui/src/index.d.ts
Outdated
| @@ -13,6 +13,7 @@ export type StandardProps<C, ClassKey extends string, Removals extends keyof C = | |||
| > & | |||
| StyledComponentProps<ClassKey> & { | |||
| className?: string; | |||
| ref?: C extends { ref?: infer RefType } ? RefType : React.Ref<unknown>; | |||
There was a problem hiding this comment.
This assumes that the component forwards the ref.
eps1lon
commented
Apr 4, 2019
| @@ -6,7 +6,7 @@ export interface BadgeProps | |||
| children: React.ReactNode; | |||
| badgeContent?: React.ReactNode; | |||
| color?: PropTypes.Color | 'error'; | |||
| component?: React.ElementType<BadgeProps>; | |||
| component?: React.ElementType<React.HTMLAttributes<HTMLDivElement>>; | |||
There was a problem hiding this comment.
See commit description of 1f2505a
eps1lon
force-pushed
the
feat/core/ref-types
branch
from
5c47e11
to
e9f0390
Compare
|
Just a heads up that I'm very busy with work right now and it may be a while before I'll have cycles to look at this. I hadn't heard about the union distribution perf regression, is there a TS issue for that? |
|
This was referenced Apr 10, 2019
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Types ref for all components not using generic props as
unknownFinishes on point in #14415
rationale
See https://github.com/eps1lon/typescript-react-ref-issue/blob/626f5197601c35a13e1207a8f4ab7e029454ecb6/index.tsx for current state of
refin React.Options
We have three options when declaring the type for the ref attribute:
unknownHTMLElementinstead ofHTMLInputElementevaluation
It allows reading as
HTMLInputElementwhile it's actually assigned toHTMLDivElement<Paper component="span" ref={divRef} />would pass but might throw.It's most likely very rare since the usual suspects for DOM escape hatches
are focus, measuring and
input.valueread/write.The bigger issue would be passing a class component which is definitely wrong
WRT to types.
EventTargetbut that type doesn't offer any value WRT measuringGiven that 1, 2 and 3 allow assigning wrong refs with
componentoverridebut only 1 allows full type safety at compile time with type narrowing we use
unknownuntil we can infer the ref type from thecomponentoverride.There's not much the compiler can do for DX at the moment. Type safety comes
from how you declare the type. We just make sure you can.
Why not just use the generic props approach?
Given that unions props won't work very good with hocs (require distributive Pick/Omit which have extremely bad perf in ts 3.4.1) it's not certain we can stick with that approach. That's why I rather not immediately apply it to the whole codebase until necessary.
It might also be better to rethink ref inference in those components given that too specific refs reject more abstract refs.
props spreading between material-ui components
Spreading props from one component to the other works flawlessly only if both are from the same "generation". One generation includes components using non-generic components (which have
Ref<unknown>) the other includes components using generic props which infer their ref type. It might also cause some issues for components with generic props if the refs mismatch or are more abstract.https://github.com/mui-org/material-ui/pull/15199/files#diff-e5a9e405d750f736e98750ee9c4a8eabR39 showcases this issue.
TODO
/cc @pelotom