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

WithStyles<typeof styles> not working properly #12334

Closed
2 tasks done
CodeJjang opened this issue Jul 30, 2018 · 18 comments
Closed
2 tasks done

WithStyles<typeof styles> not working properly #12334

CodeJjang opened this issue Jul 30, 2018 · 18 comments

Comments

@CodeJjang
Copy link

CodeJjang commented Jul 30, 2018

WithStyles<typeof styles> doesn't get along with with TS (no auto completion and I get type mismatch error).

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

Expected Behavior

New Material UI Core's WithStyles<> should be able to receive the the type of styles object somehow, for further auto-completion of classes from this.props.classes.

It works only if I hardcode the class names (i.e. WithStyles<'className'>), but it contradicts DRY principle.

Current Behavior

I tried WithStyles<typeof styles>, WithStyles<keyof typeof styles>, but nothing works.
I don't get auto-completion for the classes and also type checking fails using withStyles:

const styles = (theme: Theme) => createStyles({
    someClass: 
});
interface Props extends WithStyles<typeof styles> {
   prop: string
}
class SomeClass extends React.Component<Props> {... }

export default withStyles(styles)<Props>(SomeClass);

Tried also without createStyles or just withStyles(styles)(SomeClass), but nothing works.

Using WithStyles<keyof typeof styles>, I get no classes auto-completion, but types match.
Using WithStyles<typeof styles>, no auto completion and type mismatch error in withStyles:

...
Type SomeClass no match for signature
'(props: Props & Partial<WithTheme> & { classes: Record<string, string>;
string? any; T: string; StyleRulesCallback<infer, K>(): any ...}'

Steps to Reproduce

Written above.

Context

I can proceed with development by hardcoding all classnames or giving up on type checking, but it's frustrating.

Your Environment

Tech Version
Material-UI v1.2.0
React v16.4.0
Typescript v2.9.2
@eps1lon
Copy link
Member

eps1lon commented Jul 30, 2018

Edit: Never mind all that nonsense.

Where do you expect autocompletion? When you call createStyles it can't possibly know what you want to define. And in the component it should know which classes are defined.

Could you provide a complete repro? Yours has currently syntax errors.

@nmenke
Copy link

nmenke commented Jul 30, 2018

WithStyles<keyof ReturnType<typeof styles>>

@eps1lon
Copy link
Member

eps1lon commented Jul 30, 2018

@nmenke I did not need to use this construct and looking at the current definiton of WithStyles https://github.com/mui-org/material-ui/blob/73b0ca703d75085721e9e764b0fe950043682b99/packages/material-ui/src/styles/withStyles.d.ts#L41-L49
this should not be necessary.

@CodeJjang
Copy link
Author

@eps1lon This is extremely weird. It works in an empty project in my laptop.
I'll try tomorrow again at office and update.

Was talking about class names completion from this.props.class. Everything works as expected on a clean create-react-app-typescript project, MUI v1.2.0, same react and typescript versions.
Will check tomorrow if I missed something.

@franklixuefei
Copy link
Contributor

Okay, in my case, it doesn't work only when there are static members in the class.

@franklixuefei
Copy link
Contributor

franklixuefei commented Aug 4, 2018

For example, the following does not compile, complaining:

Type 'typeof MyPage' provides no match for the signature '(props: Partial<WithTheme> & { classes: Record<"blah" | "blah2" | "blah3", string>; } & { children?: ReactNode; }, context?: any): ReactElement<any> | null'.
class MyPage extends React.PureComponent<WithStyles<typeof styles>> {
  
  static getDerivedStateFromProps(props: WithStyles<typeof styles>) {...}

  render() {
    ...
    return ...;
  }
}

export default withStyles(styles)(MyPage);

whereas if you take static stuff out, it works... So weird.

@pelotom Any ideas? 🤕

BTW, it started happening when I bumped up versions of both @types/react@16.4.7 and @material-ui/core@1.4.2 to the latest. I'm using TS 2.9.2

@franklixuefei
Copy link
Contributor

Update:
I bumped down the version of @types/react to 16.4.6 and it worked. So I believe @types/react is the problem. However, I do not know why it could cause this problem, given the only change is this:

DefinitelyTyped/DefinitelyTyped#27417

@pelotom
Copy link
Member

pelotom commented Aug 4, 2018

@franklixuefei can you reproduce this in a codesandbox? I'm not able to reproduce here: https://codesandbox.io/s/2m0j23klp

@franklixuefei
Copy link
Contributor

franklixuefei commented Aug 6, 2018

@pelotom I've been trying to repro this in codesandbox, but with no luck. It seems like codesandbox is using a version of @types/react which is not latest, because even if I removed the @types/react from packages.json in codesandbox (both dependencies and devDependencies), the code still compiled. So this means codesandbox must have some sort of built-in react type definition somewhere.

However, if you set up your own environment in VSCode, with typescript@2.9.2, @types/react@16.4.7, the problem will start to occur. I've tried removing node_modules and re-installing all dependencies, but still same problem.

After some more digging, I think this is related to the latest change in @types/react (16.4.6 -> 16.4.7), which passed ComponentState type parameter to StaticLifecycle, which seems to have caused the problem. If I comment out the second parameter in my static getDerivedStateFromProps(), the problem will be gone. However, I need that parameter.

Problematic code:

import * as React from "react";
import { Theme } from "@material-ui/core/styles/createMuiTheme";
import withStyles, {
  CSSProperties,
  WithStyles
} from "@material-ui/core/styles/withStyles";

interface State {
  test: string;
  test2: number;
}

const styles = (theme: Theme) => ({
  style1: {
    position: "relative"
  } as CSSProperties
});

class Test extends React.PureComponent<WithStyles<typeof styles>, State> {
  constructor(props: WithStyles<typeof styles>) {
    super(props);
    this.state = {
      test: "test",
      test2: 2
    };
  }

  static getDerivedStateFromProps(
    props: WithStyles<typeof styles>,
    state: State // <-- If you comment this out, the error will disappear
  ): Partial<State> | null {
    return {
      test: '123'
    };
  }

  render() {
    return null;
  }
}

export default withStyles(styles)(Test); // <-- Error: Type 'typeof Test' provides no match for the signature '(props: Partial<WithTheme> & { classes: Record<"style1", string>; } & { children?: ReactNode; }, context?: any): ReactElement<any> | null'.

Really not sure why this is happening...

For now I'll just bump down to @types/react@16.4.6

Any ideas will be appreciated!

@franklixuefei
Copy link
Contributor

franklixuefei commented Aug 6, 2018

After some more digging... I found the root cause in @types/react.

According to a previous commit - DefinitelyTyped/DefinitelyTyped#27417, ComponentState, which is {}, has been explicitly passed to StaticLifecycle as the state type parameter from ComponentClass. However, in our withStyles.d.ts, we have

export default function withStyles<ClassKey extends string>(
  style: StyleRulesCallback<ClassKey> | StyleRules<ClassKey>,
  options?: WithStylesOptions<ClassKey>,
): {
  <P extends ConsistentWith<P, StyledComponentProps<ClassKey>>>(
    component: React.ComponentType<P & WithStyles<ClassKey>>,
  ): React.ComponentType<Overwrite<P, StyledComponentProps<ClassKey>>>;
};

where we don't specify a state type in React.ComponentType. This results in the state type defaulting to {}, which further constrains that the type of the state parameter of static getDerivedStateFromProps(props, state) to be {}, which makes the compiler complain, however, about something not readable.

As a proof, if I change the type of prevState from State to {}, the code compiles.

import * as React from "react";
import { Theme } from "@material-ui/core/styles/createMuiTheme";
import withStyles, {
  CSSProperties,
  WithStyles
} from "@material-ui/core/styles/withStyles";

interface State {
  test: string;
  test2: number;
}

const styles = (theme: Theme) => ({
  style1: {
    position: "relative"
  } as CSSProperties
});

class Test extends React.PureComponent<WithStyles<typeof styles>, State> {
  constructor(props: WithStyles<typeof styles>) {
    super(props);
    this.state = {
      test: "test",
      test2: 2
    };
  }

  static getDerivedStateFromProps(
    props: WithStyles<typeof styles>,
    state: {} // <-- changed from State to {}, and the code compiles
  ): Partial<State> | null {
    return {
      test: '123'
    };
  }

  render() {
    return null;
  }
}

export default withStyles(styles)(Test); 

@pelotom
Copy link
Member

pelotom commented Aug 6, 2018

@franklixuefei I see, so the definition of withStyles probably needs to change to track the state parameter; something like

export default function withStyles<ClassKey extends string>(
  style: StyleRulesCallback<ClassKey> | StyleRules<ClassKey>,
  options?: WithStylesOptions<ClassKey>,
): {
  <P extends ConsistentWith<P, StyledComponentProps<ClassKey>>, S>(
    component: React.ComponentType<P & WithStyles<ClassKey>, S>,
  ): React.ComponentType<Overwrite<P, StyledComponentProps<ClassKey>>, S>;
};

@franklixuefei
Copy link
Contributor

@pelotom I tried exactly that, but it will break the existing code, because S here does not have a default type.

I just filed a PR against @types/react. DefinitelyTyped/DefinitelyTyped#27914
There's only one change there - I changed {} to any.

@franklixuefei
Copy link
Contributor

franklixuefei commented Aug 6, 2018

@pelotom There is one more important reason why only changing withStyles.d.ts is not enough, because ComponentType does not accept a second type parameter unfortunately. (which I think is reasonable because ComponentType is a union of ComponentClass and StatelessComponent, and a StatelessComponent does not have a state by design)

So that's why I decided to just change {} to any in @types/react, because it is easy to reason about and is a very simple fix.

@pelotom
Copy link
Member

pelotom commented Aug 6, 2018

@franklixuefei I see, makes sense. Good luck with the PR!

@franklixuefei
Copy link
Contributor

FYI - DefinitelyTyped/DefinitelyTyped#27914 is now checked in. At least my issue is now resolved. 😄

@pelotom
Copy link
Member

pelotom commented Aug 14, 2018

@franklixuefei does that mean this can be closed?

@franklixuefei
Copy link
Contributor

@pelotom I believe so. I believe this issue is also related to #11693, which should have already been resolved because #12456 is checked in.

@pelotom
Copy link
Member

pelotom commented Aug 14, 2018

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants