Material-ui: Question on flow support

[wcandillon]

First things first, thank you for your amazing work on the beta. The work you guys are doing is so important and I'm amazed to see that you are getting right. You are really setting a new defacto standard for React app. Not to get ahead of myself, I hope your work will spill over React Native maybe by strongly influencing another UI library or something, we'll see.

I recently gave another shot to flow and thought that I would try it with material-ui.
But it looks like the type definition is not as rich than in TS. For instance:

const styles = (theme: Theme): StyleRules<Classes> => ({
    header: {
        backgroundColor: theme.palette.primary
    }
});

Doesn't look like Theme and StyleRules are not defined in flow.

How do develop the library? You write it in JavaScript and the TS definitions are written manually? I'm looking forward to learn more about this topic.

[oliviertassinari]

You write it in JavaScript and the TS definitions are written manually?

Yes, exactly. @rosskevin have been pushing forward the flow coverage of Material-UI. It's an ongoing effort. On the other hand. The TypeScript definitions have been primarily developed by @sebald and @pelotom.

I'm personally not convinced by the overall advantages a type system is providing (in practice), so it's not something I'm prioritizing my time on. Still, any help is welcome to increase the typing coverage!

[wcandillon]

@oliviertassinari Thks for the insight. I was really curious about the status. @sebald & @pelotom Thank you so much for the great support in TypeScript 👍🏻.

[gnapse]

I'm introducing flow to my own app source code, and as soon as I add // @flow to some file that imports something from material-ui/*, I get a bunch of errors from flow when I run yarn run flow. Is this related to this issue? If so, or regardless of that, what's the proposed way to solve this?

[rosskevin]

@gnapse https://material-ui-next.com/guides/flow/

[gnapse]

Great, thanks @rosskevin! My bad, I should have noticed that from the website, since I browse it all the time.

And to apply this to an existing app, what should be done? I tried copying over from this example app the folders flow-typed/, flow/ and the .flowconfig file. And I still get errors from flow in my app.

Here's the single file where I have activated flow:

// @flow
import React from 'react';
import { Link } from 'react-router-dom';
import { withStyles } from 'material-ui/styles';
import Button from 'material-ui/Button';

const styles = {
  fab: {
    position: 'fixed',
    bottom: 36,
    right: 36,
    color: 'white',
    zIndex: 1000,
  },
};

export type Props = {
  onClick: string | (Event => void),
  icon: string,
  classes: Object,
};

function Fab({ onClick, icon, classes }: Props) {
  const isLink = typeof onClick !== 'function';
  const buttonProps = isLink ? { component: Link, to: onClick } : { onClick };
  return (
    <Button fab color="accent" className={classes.fab} {...buttonProps}>
      <i className="material-icons">{icon}</i>
    </Button>
  );
}

export default withStyles(styles)(Fab);

As soon as I do that, when I run flow I get errors, these ones below:

$ yarn run flow
yarn run v1.3.2
$ /Users/ernesto/code/mdl/org-admin/node_modules/.bin/flow
The flow server's version didn't match the client's, so it exited.
Going to launch a new one.

Launching Flow server for /Users/ernesto/code/mdl/org-admin
Spawned flow server (pid=96678)
Logs will go to /private/tmp/flow/zSUserszSernestozScodezSmdlzSorg-admin.log
Error: node_modules/material-ui/ButtonBase/ButtonBase.js.flow:286
             v-----------
286:         <TouchRipple
287:           innerRef={node => {
288:             this.ripple = node;
...:
291:         />
             -^ React element `TouchRipple`
116: class TouchRipple extends React.Component<ProvidedProps & Props, State> {
                                               ^^^^^^^^^^^^^^^^^^^^^ undefined property `theme`. This type is incompatible with. See: node_modules/material-ui/ButtonBase/TouchRipple.js.flow:116
 83: export type InjectedProps = { classes: Object, theme: Object };
                                                           ^^^^^^ object type. See: node_modules/material-ui/styles/withStyles.js.flow:83

Error: src/components/Fab/Fab.js:27
 27:     <Button fab color="accent" className={classes.fab} {...buttonProps}>
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ React element `Button`. This type is incompatible with
 23:       React.ComponentType<RequiredProps & $Diff<$Diff<OriginalProps, ProvidedProps>, DefaultProps>>)
                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ empty. See: node_modules/react-flow-types/index.js.flow:23


Found 2 errors
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[rosskevin]

@gnapse you will have to align your flow version with the exact version of the material-ui release. Note that I am no longer using flow for my project due to https://github.com/facebook/flow/issues/5382 and am actively transitioning my project to use Typescript with material-ui

[gnapse]

Ok, thanks for the heads up and all the help. I'll see what I end up doing. Just a clarification: does this mean that material-ui won't ever have proper flow support? Is there a flow-typed library interface at least? And as a last resort, how could I tell flow to ignore material-ui completely?

[rosskevin]

We _cannot_ have proper flow support for apps since flow itself is broken. Since we cannot, not even an external libdef in flow-typed could have proper support. If we re-typed the HOCs to effectively any (as I discovered prior to #8419), you could pretend it had flow support, but we would have to do so knowing that we stopped static typing for almost everything. I found with any HOCs we were hiding 1700 flow errors! Essentially, we would have to make flow support almost useless.

I just submitted a PR to update the docs/example with a warning: https://github.com/mui-org/material-ui/pull/9311

As a last resort, simply ignore material-ui in .flowconfig. If https://github.com/facebook/flow/issues/5382 remains unresolved for any amount of time, we will likely stop adding flow shadow files to the distribution, as it just makes every flow user ignore them. We aren't sure yet, but it's not a good situation.

I went through considerable pain to get it to work here and in our apps, only to have to abandon it. I have more invested with flow and material-ui than anyone, and I do think it says something important that I have chosen to abandon it for typescript.

[gnapse]

Thanks for your time and all the information about this. It's unfortunate. The comment you cited in #9312 is also painfully revealing. I'll take a look at Typescript.

[TSMMark]

Very disappointing from Flow. Time to look at Typescript for new projects I guess 😞

[wcandillon]

I would like to follow up on this thread. Has anyone be able to use material-ui successfully with flow. I would like to switch to flow for all my projects but it looks like I need to stick with TypeScript if I want to use this library. Any feedback appreciated.

[oliviertassinari]

Has anyone be able to use material-ui successfully with flow.

@prastut has.

[prastut]

@oliviertassinari I would really love to update the flow documentation so that more people like me do not face the same questions that I had faced. That involves discussion with the stance we document, since currently we kind of support it (since we have flow examples inside the repo etc.) but we also kind of don't (which is the stance our message could show) i.e:

"Look folks, this is the reason why we dropped flow and as the core maintainers of the project, we felt it had very little ROI (this discussion is scattered here and there, which could be collated together). But the wonderful thing about the open source community is that some people are maintaining the flow typedefs. You can use this according to the given instructions," (followed by instructions)

There are a couple of problems we also would need to address:

1. There is a problem with the namespacing, and when I often have to update the typedefs, I go delete the typedefs inside flow-typed and then reinstall. Otherwise, flow-typed upgrade doesn't update MUI types.
2. Since the example inside MUI has to be constantly updated whenever flow types change, we either drop it completely with comprehensive instructions or remove flow-types from the example (so they automatically get installed to the latest version.
3. add how to install flow-types. If we fix the bug in point 1, most flow errors would stop to cease since automatically the example will have the recent flow-types. So if there is a bug, the entire projects that use flow+MUI combination would break.

I am ready to put in all the effort that is needed so that we solve it once and for all, I hate to see the core team getting bothered with flow requests. The problem is of information asymmetry which we can easily solve through docs.

[wcandillon]

@prastut @oliviertassinari Thanks for the feedback. I started to update the flow typings at https://github.com/flowtype/flow-typed/pull/2152/files. I hope that in the long run this type definition will be of good quality.

[oliviertassinari]

so they automatically get installed to the latest version

@prastut This sounds like a great idea 💯.

we felt it had very little ROI

This is something generic I feel with the typing systems. It's a personal opinion. It's not meant to deter people from using Flow or TypeScript. Ideally, it shouldn't impact how Material-UI support Flow and TypeScript.

I am ready to put in all the effort that is needed so that we solve it once and for all

We would definitely love to see that!!!

we kind of support it (since we have flow examples inside the repo etc.) but we also kind of don't (which is the stance our message could show)

I think that we can clarify it through documentation. We don't have the ambition to provide a first-class support for Flow here, like we do with TypeScript. But we won't do any effort against Flow.
We will try to dedicate some ressource to enable the Flow support here. Meaning, reviewing and accepting pull-request that goes into a good, scalable and maintainable Flow support direction.

[wcandillon]

I'm trying to build a flow type that is almost as good as the TS support but I'm looking for help because I'm not sure how to translate some of the utility TS types yet. I've put the current state at https://github.com/wcandillon/material-ui-flow.
I'm looking for someone help me with this.

[alexeyMohnatkin]

@wcandillon do you plan to support v3?

[rosskevin]

@alexeyMohnatkin this has not changed:

We don't have the ambition to provide a first-class support for Flow here, like we do with TypeScript. But we won't do any effort against Flow.

Please look at the community efforts if you are interested in flow.