Skip to content
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

[core] Type ref for components #15199

Merged
merged 5 commits into from
Apr 8, 2019
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 4, 2019

Types ref for all components not using generic props as unknown
Finishes on point in #14415

rationale

See https://github.com/eps1lon/typescript-react-ref-issue/blob/626f5197601c35a13e1207a8f4ab7e029454ecb6/index.tsx for current state of ref in React.

Options

We have three options when declaring the type for the ref attribute:

  1. Type it as unknown
  2. Type it exact
  3. Type it less specific e.g. HTMLElement instead of HTMLInputElement

evaluation

  1. has the drawback of assigning refs that don't extend the actual ref.
    It allows reading as HTMLInputElement while it's actually assigned to HTMLDivElement
  • Is dangerous if we allow the rendered component to be overridden e.g.
    <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.value read/write.
    The bigger issue would be passing a class component which is definitely wrong
    WRT to types.
  • rejects more abstract types
  • same issue as 2. WRT to soundness
  • would also reject more abstract types unless we use to to EventTarget but that type doesn't offer any value WRT measuring

Given that 1, 2 and 3 allow assigning wrong refs with component override
but only 1 allows full type safety at compile time with type narrowing we use
unknown until we can infer the ref type from the component override.

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

  • apply to all components

/cc @pelotom

@eps1lon eps1lon added new feature New feature or request typescript package: material-ui Specific to @mui/material labels Apr 4, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 4, 2019

No bundle size changes comparing 8db4931...533e32a

Generated by 🚫 dangerJS against 533e32a

This was technically incorrect since the override component
didn't receive the props used in the parents but only excess props
@@ -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>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that the component forwards the ref.

@eps1lon eps1lon requested a review from pelotom April 4, 2019 20:20
@@ -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>>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See commit description of 1f2505a

@eps1lon eps1lon marked this pull request as ready for review April 4, 2019 20:38
@pelotom
Copy link
Member

pelotom commented Apr 5, 2019

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?

@eps1lon
Copy link
Member Author

eps1lon commented Apr 5, 2019

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?

microsoft/TypeScript#30663

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants