mui/material-ui

Proposal: Add flow types with annotations

rosskevin opened this issue · 8 comments

rosskevin commented

I would like the community to consider adding flow to mui.

I've got it setup in my new project and it has already helped a ton.

Advantages

  • Compile-time type checking of defaultProps, props, state, and callback signatures
  • Reduced React.PropTypes code!
  • Bounded Polymorphism means no more passing invalid values to setState
  • Reduced testing due to compile-time signature checks
  • Automated code checking with CI (including PRs)
  • IDE code completion!
  • Reduced fear of refactoring

Curious?

Check out this cheat sheet of available flow types we will get checked for free! There are certainly more, but this is useful to peruse.

Implementation

Flow is opt-in by default, we can add it class by class. I suggest seeding it, and turning it on one file at a time, doing manual flow checking. Once comfortable and enough good examples have been set, add it to CI.

I would be happy to PR the setup and refactor a class as an example (just point me at the target)

newoga commented

I think this would be awesome. Only (and major) barrier I can think of to this currently is the refactoring a lot of our components may be getting as a result of the styling changes we're making in 0.16.x. Maybe we could start by documenting our component propTypes with flowtype when we refactor the components for style changes.

c089 commented

@rosskevin I'd be interested in working on this to deepen my understanding of flowtype and contribute a bit to material-ui. Ping me if you're up for some remote pairing.

oliviertassinari commented

@rosskevin I don't know if you have already started working on this, but I think that it would be good to do it incrementally.
The weak mode is already able to find issues without the need for annotations.

// @flow weak
rosskevin commented

I think we should start off right on the next branch. Once @nathanmarks is ready for contributions, I can work over that branch with a setup and PR.

  • 👍2
PTaylour commented

A bit off topic – but this could be a help to others using mui in flow projects.

External flow libdefs can be useful if a library does not export it's own flow bindings.

A quick and dirty port of typescript definitions for mui:

yarn add flowgen
yarn add @types/material-ui

./node_modules/.bin/flowgen ./node_modules/@types/material-ui/index.d.ts -o material-ui_v.0.16.x.flow.js

EDIT: I've now had a look at the bindings flowgen produces – they're not all that useful.

Most of the components are defined with declare module.exports: typeof <ClassName> where the class is not imported.

For now I've just found and replaced all the import errors with any. Not that useful, but better than nothing.

This is what the file looks like now

oliviertassinari commented

I believe we can close this issue now as the proposal was accepted 😄 .

oliviertassinari commented

@FDiskas Is right. We have documented the migration of the flow support from Material-UI to flow-typed: https://material-ui.com/guides/flow/#flow-typed.