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

[typescript] Make StyledComponent only a type, not a class #8366

Merged
merged 2 commits into from
Sep 25, 2017

Conversation

pelotom
Copy link
Member

@pelotom pelotom commented Sep 24, 2017

In the current TypeScript typings, StyledComponent is defined as a class which extends React.Component, which doesn't reflect the reality of the underlying JS code. This leads one to believe that one can extend StyledComponent as if it were a class, which it's not:

import { StyledComponent } from 'material-ui'

class Foo extends StyledComponent {
  render() { return <div>hi</div> }
}

<Foo />

The above will pass the type checker but lead to a runtime error, because no value StyledComponent is actually exported from material-ui.

By extension, all the material-ui components are defined as being classes which extend this base class, but

  1. many of them are just functional components, not classes at all
  2. even the ones which are classes directly extend React.Component, not StyledComponent which is not actually a class.

This PR fixes the typing of StyledComponent by declaring it to just be a type, not a class, and changes all components to not extend that type, but simply to have that type.

@oliviertassinari
Copy link
Member

This was quick!

@pelotom
Copy link
Member Author

pelotom commented Sep 24, 2017

@oliviertassinari Regex find/replace is my friend 😆

@vyrotek
Copy link

vyrotek commented Sep 25, 2017

This might be a dumb question. Given the example below (extend from the test in the PR) is this going to be the right way to expose multiple classes?

Is specifying 'root' | 'other' | (etc...) on StyledComponentProps<'root' | 'other'> the only way to expose each class defined on the styles? Or am I misunderstanding some TS or JSS concept?

const styles = (theme: {}) => ({
    root: {
        color: 'aqua'
    },
    other: {
        backgroundColor: 'red'
    }
});

@withStyles(styles)
class DecoratedComponent extends React.Component<NonStyleProps & StyledComponentProps<'root' | 'other'>> {
  render() {
    const { classes, text } = this.props;
    return (
      <div className={classes!.root}>
        {text}
        <div className={classes!.other}>
           {text}
        </div>
      </div>
    );
  }
}

@pelotom
Copy link
Member Author

pelotom commented Sep 25, 2017

Yes, I’d say that’s the best way to do it currently.

@sebald
Copy link
Member

sebald commented Sep 25, 2017

Thx for fixing that, this was some debt, when the StyledComponent wasn't exposed publicly!

@sebald sebald merged commit 1a146b8 into mui:v1-beta Sep 25, 2017
@pelotom pelotom deleted the ts-fix-styled-component branch September 25, 2017 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants