Material-ui: [typescript] WithStyles<typeof styles> makes theme required

[geirsagberg]

• [x] This is not a v0.x issue.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

type Props = WithStyles<typeof styles> or WithStyles<typeof styles, undefined> should not have theme as required.

Current Behavior

type Props = WithStyles<typeof styles> has theme as required;

Steps to Reproduce

Link: https://github.com/geirsagberg/material-bug

Context

The problem appeared after upgrading to 3.1.0. Cannot seem to reproduce in Codesandbox, only locally. Clone the mentioned repo to reproduce.

Your Environment

| Tech | Version |
|--------------|---------|
| Material-UI | v3.1.0 |
| React | v16.5.1 |
| Browser | - |
| TypeScript | 3.0.3 |
| etc. | |

[dotbear]

This appears to be related to the work done here https://github.com/mui-org/material-ui/pull/12673

[eps1lon]

@dotbear I'm very sorry that this caused so many issues for you. Aside from the issue with theme it should've been as simple as

- export const MyComponent = decorate<Props>(props => <Button />)
+ export const MyComponent = decorate((props: Props) => <Button />)

where Props extends WithStyles.

Type definitions and semver don't really mix very well. The definitions over at DefinitelyTyped don't follow semver at the moment and I believe that this is ok. Mixing the definitions into the package can cause problems across minor versions. The overall problem is that the old withStyles did not support defaultProps and could've been considered a bug that required a breaking change. From my experience with type definitions this happens very often and would require a frequent bump in major versions which might leave non-ts users behind.

In retrospect we could've simply added a withStylesLibraryManaged that supported defaultProps. This would've allowed users to get typed defaultProps with the new method while we could still support legacy code bases with the inferior withStyles.

I would suggest using @ts-ignore if this is possible. The code will emit just fine. It's just much noise in the form of false positives.

[eps1lon]

@geirsagberg It's hard for me to reproduce. I had vscode show me the same issue with theme required but after switching between ts versions it disappeared.

Your repro is also incomplete. You are not actually decorating the component with withStyles. It would be very helpful if you could provide a repro that includes compile task that throws.

Overall if this actually throws when compiling this looks like an issue with typescript. I don't see how undefined extends true should evaluate to true while false extends true evaluates to false.

[dotbear]

@eps1lon try this one https://github.com/dotbear/material-ui-bug

[eps1lon]

@dotbear After adding the following tsconfig and running tsc -p tsconfig.json --noEmit I got no errors.

I got the error however if I did not add es5 and es6 to the lib which caused ts to not compile the type definitions of mui.

Please provide a repro that includes the error on compilation. Otherwise this is either an issue with vscode or your tsconfig.

[geirsagberg]

@eps1lon I have updated the repo now. I agree that this seems to be an issue with TypeScript. I will do some more testing to try to isolate the issue, seems related to what is in the tsconfig.

[geirsagberg]

@eps1lon The issue occurs on compilation when "strict": false.

[geirsagberg]

Repo updated, running tsc --noEmit fails.

[dotbear]

@eps1lon good point.

I did some testing and I've isolated the problem. It occurs when strict: false in your tsconfig. Which means this will only work with strict: true.

[eps1lon]

More specifically strictNullChecks: false will cause errors. Setting this to true will get rid of the errors. Since DefinitelyTyped also requires strictNullChecks: true I think we can also require this too.

The issue is described in https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html

The type checker previously considered null and undefined assignable to anything

Which is the issue here.

Again I'm sorry for the inconvenience but this was always a potential error in codebases that did not set strictNullChecks and I understand that combining this with our judgement call can be very frustrating.

[geirsagberg]

So the workaround is to set strictNullChecks: true, unfortunately that caused roughly 400 errors in our codebase, so I will have to downgrade to 3.0.3 for now, until we have time to strictify our codebase.

However, if I change the declaration to:

export type WithStyles<
  T extends string | StyleRules | StyleRulesCallback = string,
  IncludeTheme extends boolean = false
> = (IncludeTheme extends true ? { theme: Theme } : {}) & {
  classes: ClassNameMap<
    T extends string
      ? T
      : T extends StyleRulesCallback<infer K> ? K : T extends StyleRules<infer K> ? K : never
  >;
  innerRef?: React.Ref<any> | React.RefObject<any>;
};

Everything seems to work as expected even with strictNullChecks: false. Haven't run the tests, but do you see any issues with this?

If that is not a valid solution, I think it would be good to note this as a breaking change, or at least add it to the TypeScript bit of the documentation. I understand that it is challenging to keep supporting all different kinds of configurations, I was just taken aback by this sudden problem, and I imagine there are others with the same problem.

[eps1lon]

Which leads us to a discussion what's a breaking change and a bug fix. With strictNullChecks: false and @material-ui/[email protected] you could write:

const Child = withStyles(childStyles)<ChildProps>(({ classes, foo, theme }) => (
  <div className={classes.root}>{foo}{theme.typography.display4}</div>
));

which would pass at compile time but throw at runtime because theme is never injected and therefore undefined.

Setting strictNullChecks: false is not only potentially dangerous because it's intrinsic value but also because every type definition from DefinitelyTyped you have in your codebase is probably broken.

[geirsagberg]

I understand that strictNullChecks: true is safer, but with my suggested declaration above, you will correctly get the error in your code even with strictNullChecks: false, and we can give a warning/heads up that MUI will start to require strictNullChecks: true in a future version.

The problem here was that strictNullChecks: false worked perfectly fine in 3.0.3 and prior, and no longer worked in 3.1.0, without the release notes mentioning this at all.

If you're up for it I can make a PR with my suggested changes and make sure it passes the current tests?

[eps1lon]

The problem here was that strictNullChecks: false worked perfectly fine in 3.0.3 and prior, and no longer worked in 3.1.0, without the release notes mentioning this at all.

It did not as my example pointed out. It only worked if you consider passing at compile time while throwing at runtime as "working". I will file a PR that includes your fix and a bit of documentation but saying that it was working with strictNullChecks disabled is wrong. DefinitelyTyped is not documenting this as well which is an issue but documenting a requirement is (in my opinion) not a breaking change.

It's very convenient that we can apply a fix that is compliant with both configurations but we never have supported strictNullChecks disabled and neither is DefinitelyTyped so I don't see a reason that we should.

I can already see the issues were people complain that accessing theme at compile time is fine while it throws at runtime and that we should fix this which we can't because some people were relying on a bug.

[dotbear]

@eps1lon woah that's pretty interesting. I had to go check that myself and you're right, that does indeed compile. I was skeptical because I get correct errors for random props like bar, but it does indeed think theme is injected.

This changes everything for me. I see the issue and I understand where you're coming from. We'll have to stay on 3.0.3 until we get around to finally fixing all the null checking (legacy app).

If you could update the typescript docs to let people know about this, then that should do the trick in my book.

Thanks for your time! :)

[oliviertassinari]

A fix was merged. It should be good in v3.1.1 (this weekend). Let us know :)