[typescript] Fix with* injectors ignoring defaultProps #12673

eps1lon · 2018-08-27T14:17:13Z

withStyles, withWidth and withTheme now preserves defaultProps information.

The definitions are provided by DefinitelyTyped/DefinitelyTyped#28189.

This is a breaking change for typescript users that provided type arguments for the injectors or called the injectors with class or function expressions rather than with previously declared (and typed) variables. Looking at the use cases I had to change I think this is ok. The guides never used those patterns and this enforces a (in my opinion) cleaner component declaration

AnyComponent was removed since it was no longer used. PropsOf can't user Rect.ComponentType for inference though since this breaks union types.

Fixes: #12670

eps1lon · 2018-08-27T16:05:53Z

Thought reopening triggers CI which is not the case. Rebased with master to trigger CI. Failures where unrelated.

pelotom · 2018-08-30T21:42:55Z

packages/material-ui/test/typescript/styles.spec.tsx

+        );
+      }
+    },
+  );


There's already a test for union props in stylingComparison.spec.tsx.

pelotom · 2018-08-30T22:38:32Z

packages/material-ui/test/typescript/styles.spec.tsx

+  // https://github.com/mui-org/material-ui/issues/12670
+  interface Props {
+    noDefault: string;
+    withDefaultProps: number;


Can we call these props nonDefaulted and defaulted respectively?

pelotom · 2018-08-30T22:46:41Z

packages/material-ui/src/index.d.ts

+ */
+export type Shared<
+  InjectedProps,
+  DecorationTargetProps extends Shared<InjectedProps, DecorationTargetProps>


It's confusing that InjectedProps and DecorationTargetProps are in the opposite order here vs. in Matching.

This self-referential constraint is bending my mind: DecorationTargetProps extends Shared<InjectedProps, DecorationTargetProps>. What's the rationale for this? Removing it doesn't break any tests, which makes me suspect it's extraneous.

pelotom · 2018-08-30T22:58:11Z

packages/material-ui/src/styles/withStyles.d.ts

+  Omit<
+    JSX.LibraryManagedAttributes<C, PropsOf<C>>,
+    keyof Shared<WithStyles<ClassKey, Options['withTheme']>, PropsOf<C>>
+  > &


We only ever use keyof Shared<...>? This makes me think we can get rid of Shared completely and just use

Omit< JSX.LibraryManagedAttributes<C, PropsOf<C>>, (keyof WithStyles<ClassKey, Options['withTheme']>) & (keyof PropsOf<C>) >

here. Furthermore the & (keyof PropsOf<C>) seems redundant, and it can just be

Omit< JSX.LibraryManagedAttributes<C, PropsOf<C>>, keyof WithStyles<ClassKey, Options['withTheme']> >

pelotom · 2018-08-30T23:28:40Z

packages/material-ui/src/index.d.ts

+  [P in keyof DecorationTargetProps]: P extends keyof InjectedProps
+    ? InjectedProps[P] extends DecorationTargetProps[P] ? DecorationTargetProps[P] : never
+    : DecorationTargetProps[P]
+};


It seems like Matching is meant to accomplish much the same as ConsistentWith, but maybe does a better job? Do we still need ConsistentWith or can Matching replace it everywhere?

I thought so too but ConsistentWith did not help with this issue. Maybe replace ConsistentWith in a future PR with Matching or explore where they actually differ. Same goes for AnyComponent.

eps1lon · 2018-08-31T00:19:58Z

@pelotom The requested changes made the definitions much easier to follow. Thank you.

pelotom · 2018-08-31T04:21:50Z

packages/material-ui/test/typescript/styles.spec.tsx

+
+  class Component extends React.Component<Props & WithStyles<typeof styles>> {}
+  // $ExpectError
+  const StyledComponent = withStyles(styles)(Component);


This consistency check isn't working when applied to an SFC:

// $ExpectError withStyles(styles)((props: Props) => null);

@pelotom This is a limitation of the never type. I cannot assign T to never but a function that takes T to a function that takes never. typescript playground

If I type this function explicitly as a React.SFC I do get errors but the message is confusing:
Property 'children' is missing in type 'ValidationMap<Props>'.

I'm going to verify if this was working before. If we can't resolve this we have to make a decision whether we want to support defaultProps or the consistency check for stateless components.

It did indeed work.

I would prefer defaultProps support over consistency checks for implicit SFCs. We could always add a section to the typescript guide.

Hm, are you 100% sure that we can't support both? I haven't looked at this in enough detail to know... but I agree that defaultProps takes precedence if it has to be one or the other.

Not with the never approach. That's either an issue with ts (although I couldn't really find reports on this) or an actual limitation. I tried using the previous approach with ConsistentWith but either ts inferred {} as the prop type or I got errors when passing a styled component to withTheme

I probably revisit the react-redux typings since I used the Matching from there and see if I can connect inconsistent props with their typings too. That way we get more focus on the issue and maybe get some more help.

But as is the issue with #12697 this problem at least has a workaround with explicit typing.

If we can't do a complete job of enforcing consistency maybe we should just remove the constraints completely, as they significantly complicate the expression of the types, and arguably they're of fairly niche value.

@eps1lon as the original author on the react-redux PR, happy to help out if I can. I haven't gotten any reviews in 20 days so it's honestly been a bit hard to iterate/improve on the definition. If there are edge cases it doesn't handle, I can certainly iterate.

I also have my email in my github profile so feel free to reach out that way.

FWIW, is the issue you're dealing with related to this by any chance DefinitelyTyped/DefinitelyTyped#28249?

That issue is probably causing the fail with the explicitly typed SFC. So if that issue is resolved we might not even have a consistency check for explicitly typed SFCs.

eps1lon · 2018-09-06T07:08:40Z

Got it to work with a little gotcha that was already happening before.

Before this change we returned a component that had an optional theme prop although it might've been required by the props definition. See https://codesandbox.io/s/6zzjk115jr. Everything is good at compile time but obviously we never injected the theme so we get undefined at runtime.

The issue is basically that the Matching type does not handle undefined well since we can't assign undefined | T to T. Maybe there is a better Matching but I can't think of any at the moment.

pelotom · 2018-09-06T17:42:40Z

This is great! Now it seems like we can define

export type ConsistentWith<T, U> = Matching<U, T>;

and everything works. So why not just redefine ConsistentWith and get rid of Matching?

eps1lon · 2018-09-06T18:37:26Z

I'll take a look into that. I'm also looking at the other injector helpers (`withTheme` etc.).

…

On Thu, Sep 6, 2018, 19:43 Tom Crockett ***@***.***> wrote: This is great! Now it seems like we can define export type ConsistentWith<T, U> = Matching<U, T>; and everything works. So why not just redefine ConsistentWith and get rid of Matching? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12673 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALuPz81itbysbk-C_q9u5GuXE_xjBqCkks5uYV62gaJpZM4WN32V> .

apapirovski · 2018-09-07T02:02:33Z

packages/material-ui/src/index.d.ts

+ */
+export type Matching<InjectedProps, DecorationTargetProps> = {
+  [P in keyof DecorationTargetProps]: P extends keyof InjectedProps
+    ? InjectedProps[P]


This no longer validates that the two interfaces passed in are matching. If InjectedProps[P] doesn't extend DecorationTargetProps[P] (assuming their keys are shared) then that means that the two definitions are incompatible and cannot be reconciled. E.g., this would accept: Matching<{ test: string; }, { test: boolean; }>

Edit: actually, you can ignore me, I think I see what you're doing? Maybe... Need to test it locally.
Edit2: Nope, this is definitely incorrect but not for the reason I mentioned. The problem is that if your component declares something as optional and the InjectedProps spec something as required then it won't match.

@apapirovski
Which is sufficient for usage in withStyles. The previous version allowed mismatch of props for functional components that did not have the React.SFC type or to be more specific only had a call signature. I'm going to checkout the react/redux typings with your PR and see if this happens there to.

I posted a new definition for Matching in my PR that is IMO slightly more correct, at least as far as connect goes. Not sure if you have different requirements here as I don't use material-ui. Thanks for the test case and the triage!

@apapirovski
I agree that approach is indeed better and I tried this before but this caused some regression. I realized that this was not an issue with your definition but with ours. Basically injecting T | undefined while we actually only inject T if it is defined which is different when using spread syntax.

eps1lon · 2018-09-08T08:04:34Z

@pelotom
I refactored the definitions to use a common PropInjector type. All with* injectors now support default props. Again this is a breaking change because it does not allow automatic type inference of the props. The other change is that the props can no longer passed as the generic argument but this is trivial since the type annotation just has to be moved.

We can batch this for v4 since defaultProps were never working before @types/react introduced LibraryManagedAttributes.

pelotom

Again this is a breaking change because it does not allow automatic type inference of the props.

Just to make sure I'm clear on this, you mean the fact that you can no longer write

withStyles(styles)<Props>(props => ...);

and instead have to write

withStyles(styles)((props: Props) => ...);

?

pelotom · 2018-09-11T16:04:02Z

This all looks good to me. I would defer to @oliviertassinari about whether to wait for v4 to release this. In the past we have on occasion made backwards incompatible changes to the typings without revving a major version 🤷‍♂️

eps1lon · 2018-09-11T16:12:36Z

Yes. The changed tests illustrate theses changes.

…

On Tue, Sep 11, 2018, 18:00 Tom Crockett ***@***.***> wrote: ***@***.**** commented on this pull request. Again this is a breaking change because it does not allow automatic type inference of the props. Just to make sure I'm clear on this, you mean the fact that you can no longer write withStyles(styles)<Props>(props => ...); and instead have to write withStyles(styles)((props: Props) => ...); ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12673 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALuPz-y-Hbvl0WJ9wjA2fZJB4mLJ_ZHMks5uZ94ngaJpZM4WN32V> .

oliviertassinari · 2018-09-11T18:44:55Z

It would be great to add a breaking change summary in the pull request description. If it's breaking a popular pattern, I think that we should wait v4. If its an edge case, I think that releasing it in the next minor is "OK".

Can confirm. The definitions for react-redux have the same issue (e.g. DefinitelyTyped/DefinitelyTyped#27959) and basically every other hoc definition.

Makes me believe people are already used to work around the problem.

pelotom · 2018-09-11T18:50:03Z

If its an edge case, I think that releasing it in the next minor is "OK".

My guess is that we'd be breaking a fairly niche use case, and it's pretty straightforward to fix. Meanwhile, the upside of being able to use defaultProps is pretty strong. So I'd vote for pushing ahead with the change now.

pelotom · 2018-09-11T20:36:33Z

Thanks for all your hard work @eps1lon!

oliviertassinari · 2018-09-11T20:37:12Z

Will be released in v3.1.0.

* [core] Fix withStyles ignoring defaultProps * [typescript] Fix chore review issues * [typescript] Normalize order of InjectedProps and TargetProps * [typescript] Remove extraneous type constraint in Shared * [typescript] Removed extraneous Shared type * [typescript] Add more test cases for stateless componets * [typescript] Fix false positive consistency check for implicit SFCs * [typescript] Removed unused prop * [typescript] Cleanup component tests * [typescript] Fix wrong injected theme type in withStyles * [typescript] Introduce generic prop injector HOC

eps1lon changed the title ~~[core] Fix withStyles ignoring defaultProps~~ [typescript] Fix withStyles ignoring defaultProps Aug 27, 2018

oliviertassinari added the typescript label Aug 27, 2018

eps1lon closed this Aug 27, 2018

eps1lon reopened this Aug 27, 2018

eps1lon force-pushed the fix-with-styles-default-props branch from efefed7 to 0c7073d Compare August 27, 2018 16:05

rosskevin requested a review from pelotom August 30, 2018 21:16

pelotom suggested changes Aug 30, 2018

View reviewed changes

pelotom suggested changes Aug 31, 2018

View reviewed changes

eps1lon added 7 commits September 6, 2018 09:09

[core] Fix withStyles ignoring defaultProps

5c36cb0

[typescript] Fix chore review issues

d6d88bb

[typescript] Normalize order of InjectedProps and TargetProps

25b333b

[typescript] Remove extraneous type constraint in Shared

a10f121

[typescript] Removed extraneous Shared type

917f221

[typescript] Add more test cases for stateless componets

93da0df

[typescript] Fix false positive consistency check for implicit SFCs

5da60a0

eps1lon force-pushed the fix-with-styles-default-props branch from 17dc556 to 5da60a0 Compare September 6, 2018 07:09

eps1lon added 2 commits September 6, 2018 09:17

[typescript] Removed unused prop

8c83e14

[typescript] Cleanup component tests

eed5c48

apapirovski reviewed Sep 7, 2018

View reviewed changes

eps1lon added 2 commits September 8, 2018 09:13

[typescript] Fix wrong injected theme type in withStyles

859d2d9

[typescript] Introduce generic prop injector HOC

a0ffe02

eps1lon changed the title ~~[typescript] Fix withStyles ignoring defaultProps~~ [typescript] Fix with* injectors ignoring defaultProps Sep 8, 2018

pelotom reviewed Sep 11, 2018

View reviewed changes

pelotom approved these changes Sep 11, 2018

View reviewed changes

oliviertassinari approved these changes Sep 11, 2018

View reviewed changes

pelotom merged commit c164b9c into mui:master Sep 11, 2018

dotbear mentioned this pull request Sep 17, 2018

[typescript] WithStyles<typeof styles> makes theme required #12891

Closed

2 tasks

eps1lon deleted the fix-with-styles-default-props branch September 22, 2018 21:25

eps1lon mentioned this pull request Sep 27, 2018

[typescript] Style typing improvements #12492

Merged

eps1lon mentioned this pull request Oct 17, 2018

TS typings are not working for styled components #13167

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript] Fix with* injectors ignoring defaultProps #12673

[typescript] Fix with* injectors ignoring defaultProps #12673

eps1lon commented Aug 27, 2018 •

edited

Loading

eps1lon commented Aug 27, 2018

pelotom Aug 30, 2018

pelotom Aug 30, 2018

pelotom Aug 30, 2018

pelotom Aug 30, 2018

pelotom Aug 30, 2018

eps1lon Aug 31, 2018

eps1lon commented Aug 31, 2018

pelotom Aug 31, 2018

eps1lon Sep 4, 2018

eps1lon Sep 5, 2018

pelotom Sep 5, 2018

eps1lon Sep 5, 2018

pelotom Sep 5, 2018 •

edited

Loading

apapirovski Sep 5, 2018

apapirovski Sep 5, 2018

eps1lon Sep 6, 2018

eps1lon commented Sep 6, 2018

pelotom commented Sep 6, 2018

eps1lon commented Sep 6, 2018 via email

apapirovski Sep 7, 2018 •

edited

Loading

eps1lon Sep 7, 2018

apapirovski Sep 7, 2018 •

edited

Loading

eps1lon Sep 7, 2018

eps1lon commented Sep 8, 2018

pelotom left a comment

pelotom commented Sep 11, 2018

eps1lon commented Sep 11, 2018 via email

oliviertassinari commented Sep 11, 2018

pelotom commented Sep 11, 2018

pelotom commented Sep 11, 2018

oliviertassinari commented Sep 11, 2018

[typescript] Fix with* injectors ignoring defaultProps #12673

[typescript] Fix with* injectors ignoring defaultProps #12673

Conversation

eps1lon commented Aug 27, 2018 • edited Loading

eps1lon commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelotom Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Sep 6, 2018

pelotom commented Sep 6, 2018

eps1lon commented Sep 6, 2018 via email

apapirovski Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Sep 8, 2018

pelotom left a comment

Choose a reason for hiding this comment

pelotom commented Sep 11, 2018

eps1lon commented Sep 11, 2018 via email

oliviertassinari commented Sep 11, 2018

pelotom commented Sep 11, 2018

pelotom commented Sep 11, 2018

oliviertassinari commented Sep 11, 2018

eps1lon commented Aug 27, 2018 •

edited

Loading

pelotom Sep 5, 2018 •

edited

Loading

apapirovski Sep 7, 2018 •

edited

Loading

apapirovski Sep 7, 2018 •

edited

Loading