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 the classes property nullable in withStyles #8310

Closed
wants to merge 3 commits into from
Closed

[typescript] Make the classes property nullable in withStyles #8310

wants to merge 3 commits into from

Conversation

ssalbdivad
Copy link

Problem:
The classes prop seems to be nullable elsewhere but not yet in withStyles, forcing components that consume withStyles to pass a classes prop.

Solution:
Make classes nullable in #withStyles.

Resolves #8267

@ssalbdivad ssalbdivad changed the title [withStyles ]Allow the classes property to be nullable in withStyles. [withStyles] Allow the classes property to be nullable in withStyles. Sep 21, 2017
@ssalbdivad ssalbdivad changed the title [withStyles] Allow the classes property to be nullable in withStyles. [withStyles] Make the classes property nullable in withStyles. Sep 21, 2017
@oliviertassinari
Copy link
Member

Also, could you add a test with your use case under this file: https://github.com/callemall/material-ui/blob/914b8bec96cbc8d04552b5e66dc9acbc99616636/test/typescript/components.spec.tsx so we don't introduce future regression. Thanks

@oliviertassinari oliviertassinari changed the title [withStyles] Make the classes property nullable in withStyles. [typescript] Make the classes property nullable in withStyles Sep 21, 2017
@ssalbdivad
Copy link
Author

Will add a test tonight, thanks for pointing me in the right direction!

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Sep 21, 2017
@pelotom
Copy link
Member

pelotom commented Sep 22, 2017

I think #8320 solves this problem as well.

@ssalbdivad
Copy link
Author

I still don't feel this is quite right. It seems like ideally the classes prop is optionally passed down but should not be null by the time you are in the element itself so it can be safely referenced. The approach from this CR works but I have to specify something like className={classes ? classes.root : ""} in my TSX to avoid a complain from TS if strict null checks is on, which is also kind of gross. Maybe because I'm new to the ecosystem in general I just don't have a lot of clarity or intuition around using material-ui styles with TS. Any suggestions here?

@pelotom
Copy link
Member

pelotom commented Sep 22, 2017

@ssalbdivad I think the problem you're running up against is that a correct typing of withStyles needs to mutate the type of the component, transforming it from the implementation view where classes definitely exists, to the usage view where classes doesn't need to exist (it's only an optional override). Unfortunately TypeScript decorators can't do that currently.

@pelotom
Copy link
Member

pelotom commented Sep 22, 2017

Leaving aside using withStyles as a decorator, which I think for the time being in TypeScript is a bad idea due to the aforementioned issue... if you use the typing I provide in #8320 your testcase becomes something like

import * as React from 'react'
import { withStyles, WithStyles } from 'material-ui'

const styles = (theme: {}) => ({
  root: {
    color: 'aqua',
  },
})

type NoClassesRequiredProps = WithStyles<{
  bestComponentLibrary: string
}, 'root'>

class NoClassesRequiredWrapped extends React.Component<NoClassesRequiredProps> {
  render() {
    return (
      <div className={this.props.classes.root}>
        {this.props.bestComponentLibrary}
      </div>
    )
  }
}

const NoClassesRequired = withStyles(styles)(NoClassesRequiredWrapped)

<NoClassesRequired bestComponentLibrary="MUI" />

or more simply as a stateless functional component:

const NoClassesRequired = withStyles(styles)<{ bestComponentLibrary: string }>(props => (
  <div className={props.classes.root}>
    {props.bestComponentLibrary}
  </div>
))

<NoClassesRequired bestComponentLibrary="MUI" />

@oliviertassinari
Copy link
Member

@pelotom Let's move forward with #8320. Do we have to add a new test case to your PR?
@ssalbdivad Thanks for your time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants