mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.41k stars 32.15k forks source link

Configuring flow from scratch #8073

Closed ikzjfr0 closed 7 years ago

ikzjfr0 commented 7 years ago

Problem description

flowtype check comes a lot of error

Steps to reproduce

  1. create a helloworld project via create-react-app
  2. add flow support via: flow init
  3. in terminal, type flow to check error (should no error comes)
  4. yarn add material-ui@next
  5. type flow again ,which comes a lot of error

Versions

image

rosskevin commented 7 years ago

You will likely need to match (or exceed) the flow version used by the version of material-ui you are using. Flow introduced breaking changes in both 0.53.1 and 0.54.0. You may need to configure your .flowconfig to ignore problematic libraries (in this case jss isn't problematic, but due to recent changes/release cycles there may be a mismatch). It can be somewhat of a moving target.

Here's my app's .flowconfig I'm using with the v1-beta tip:

[ignore]
# prefix with .* to get nested ignores
.*/node_modules/editions
.*/node_modules/babel-plugin-flow-react-proptypes
.*/node_modules/babel-plugin-transform-react-remove-prop-types
.*/node_modules/eslint-plugin-jsx-a11y
.*/node_modules/graphql
.*/node_modules/kefir
.*/node_modules/react-event-listener/src

# temporary - flow 54 update revealed this
.*/node_modules/jss/lib/renderers/DomRenderer.js.flow

[include]

[libs]
# local/custom libdefs
./flow
# jss flow-typed libdefs
./node_modules/jss/flow-typed

[options]
esproposal.class_static_fields=enable
esproposal.class_instance_fields=enable
esproposal.export_star_as=enable
esproposal.decorators=ignore

suppress_comment= \\(.\\|\n\\)*\\$FlowFixMe
unsafe.enable_getters_and_setters=true
ikzjfr0 commented 7 years ago

Thanks. By the way, my problem existed in flow 0.50.0 and 0.54.1 (i didn't check that between these two versions)

ikzjfr0 commented 7 years ago

@rosskevin Now I'm on flow 0.54.1 and included your code snippet, following errors come after running flow status

image

rosskevin commented 7 years ago

Is that pulled directly from the repo? I'll look at it in an hour or so.

rosskevin commented 7 years ago

@ikzjfr0 you are using a new flow-bin and an old (not updated) codebase. You may either use the latest material-ui or match the flow-bin in the material-ui/package.json you are using.

This is the latest signature that conforms with 0.53.1+ class BottomNavigationButton extends React.Component<AllProps> {

zachwolf commented 7 years ago

Going to tag onto this thread. Apologies if this isn't the right place.

I'm trying to add flow to an existing project. I'm new to flow, so maybe I'm missing something obvious, but I've been reading through the docs and forums for the past few days and still not getting anywhere.

I worked through the original errors posted above by ignoring those specific files. So now my .flowconfig looks like:

[ignore]
<PROJECT_ROOT>/node_modules/material-ui/styles/withStyles.js.flow
<PROJECT_ROOT>/node_modules/react-scrollbar-size/node_modules/react-event-listener/.*
<PROJECT_ROOT>/node_modules/jss/lib/renderers/DomRenderer.js.flow
<PROJECT_ROOT>/node_modules/jss/lib/rules/ConditionalRule.js.flow
<PROJECT_ROOT>/node_modules/jss/lib/rules/FontFaceRule.js.flow
<PROJECT_ROOT>/node_modules/jss/lib/rules/KeyframesRule.js.flow
<PROJECT_ROOT>/node_modules/jss/lib/rules/StyleRule.js.flow
<PROJECT_ROOT>/node_modules/jss/lib/rules/ViewportRule.js.flow
<PROJECT_ROOT>/node_modules/jss/lib/rules/SimpleRule.js.flow
<PROJECT_ROOT>/node_modules/jss/lib/types.js.flow
<PROJECT_ROOT>/node_modules/babel-plugin-transform-react-remove-prop-types/.*
<PROJECT_ROOT>/build/.*
<PROJECT_ROOT>/scripts/.*
<PROJECT_ROOT>/coverage/.*
<PROJECT_ROOT>/config/.*
.*\.test\.js

[include]

[libs]
<PROJECT_ROOT>/flow-typed/.*

[lints]

[options]
emoji=true

# jss uses getters, supress related warnings
unsafe.enable_getters_and_setters=true

And then the console outputs:

Launching Flow server for /Users/XXXX/Sites/react-test-components
Spawned flow server (pid=11333)
Logs will go to /private/tmp/flow/zSUserszSzXXXXzSSiteszSreact-test-components.log
No errors!

My issue now is that MUI's .js.flow still don't seem to have any effect. For example, in Button.js.flow the fab prop is defined like fab?: boolean, but creating a component with <Button fab="test" /> flow still reports No errors!.

Any guidance on getting this to work?

rosskevin commented 7 years ago

@zachwolf did you use the .flowconfig I posted? Did you read my comment? https://github.com/callemall/material-ui/issues/8073#issuecomment-327636457

It's a bit frustrating to take time to post a from scratch instructions and have others post problems in the same exact thread without reading/trying it.

Most of your ignores are incorrect/verbose/unnecessary, you don't need to include flow-typed (it's automatic unless it is a nested one).

zachwolf commented 7 years ago

@rosskevin, yeah, sorry for the confusion. To clarify, I did try yours first. It still didn't give me what I was expecting. With that .flowconfig I get:

Launching Flow server for /Users/XXXX/Sites/react-test-components
Spawned flow server (pid=12314)
Logs will go to /private/tmp/flow/zSUserszSzXXXXzSSiteszSreact-test-components.log
Error: node_modules/material-ui/styles/withStyles.js.flow:32
 32: export const sheetsManager = new Map();
                                  ^^^^^^^^^ type parameter `K` of constructor call. Missing annotation

Error: node_modules/material-ui/styles/withStyles.js.flow:32
 32: export const sheetsManager = new Map();
                                  ^^^^^^^^^ type parameter `V` of constructor call. Missing annotation

Found 2 errors

It still doesn't catch the misused fab prop.

I'll delete the flow-typed. Reading through flow's docs it I understood it to be needed.

Without the ignores, I get this output:

Error: node_modules/babel-plugin-transform-react-remove-prop-types/src/index.js:83
 83:             const plugin = require(pluginName);
                                ^^^^^^^^^^^^^^^^^^^ The parameter passed to require() must be a literal string.

Error: node_modules/jss/lib/renderers/DomRenderer.js.flow:15
 15: function getStyle(rule: HTMLElement|CSSStyleRule, prop: string): string {
                                         ^^^^^^^^^^^^ identifier `CSSStyleRule`. Could not resolve name

Error: node_modules/jss/lib/renderers/DomRenderer.js.flow:51
 51: function getSelector(rule: CSSOMRule): string {
                                ^^^^^^^^^ identifier `CSSOMRule`. Could not resolve name

Error: node_modules/jss/lib/renderers/DomRenderer.js.flow:128
128:       return node
                  ^^^^ Node. This type is incompatible with
123: function findCommentNode(text: string): Comment|null {
                                             ^^^^^^^^^^^^ union: Comment | null
  Member 1:
  123: function findCommentNode(text: string): Comment|null {
                                               ^^^^^^^ Comment
  Error:
  128:       return node
                    ^^^^ Node. This type is incompatible with
  123: function findCommentNode(text: string): Comment|null {
                                               ^^^^^^^ Comment
  Member 2:
  123: function findCommentNode(text: string): Comment|null {
                                                       ^^^^ null
  Error:
  128:       return node
                    ^^^^ Node. This type is incompatible with
  123: function findCommentNode(text: string): Comment|null {
                                                       ^^^^ null

Error: node_modules/jss/lib/rules/ConditionalRule.js.flow:19
 19:   renderable: ?CSSStyleRule
                    ^^^^^^^^^^^^ identifier `CSSStyleRule`. Could not resolve name

Error: node_modules/jss/lib/rules/FontFaceRule.js.flow:16
 16:   renderable: ?CSSStyleRule
                    ^^^^^^^^^^^^ identifier `CSSStyleRule`. Could not resolve name

Error: node_modules/jss/lib/rules/KeyframesRule.js.flow:19
 19:   renderable: ?CSSStyleRule
                    ^^^^^^^^^^^^ identifier `CSSStyleRule`. Could not resolve name

Error: node_modules/jss/lib/rules/SimpleRule.js.flow:15
 15:   renderable: ?CSSStyleRule
                    ^^^^^^^^^^^^ identifier `CSSStyleRule`. Could not resolve name

Error: node_modules/jss/lib/rules/StyleRule.js.flow:19
 19:   renderable: ?CSSStyleRule
                    ^^^^^^^^^^^^ identifier `CSSStyleRule`. Could not resolve name

Error: node_modules/jss/lib/rules/ViewportRule.js.flow:16
 16:   renderable: ?CSSStyleRule
                    ^^^^^^^^^^^^ identifier `CSSStyleRule`. Could not resolve name

Error: node_modules/jss/lib/types.js.flow:26
 26:   setStyle(rule: HTMLElement|CSSStyleRule, prop: string, value: string): boolean;
                                  ^^^^^^^^^^^^ identifier `CSSStyleRule`. Could not resolve name

Error: node_modules/material-ui/styles/withStyles.js.flow:32
 32: export const sheetsManager = new Map();
                                  ^^^^^^^^^ type parameter `K` of constructor call. Missing annotation

Error: node_modules/material-ui/styles/withStyles.js.flow:32
 32: export const sheetsManager = new Map();
                                  ^^^^^^^^^ type parameter `V` of constructor call. Missing annotation

Error: node_modules/react-scrollbar-size/node_modules/react-event-listener/src/index.js:49
 49:   children?: React.Element<any>,
                  ^^^^^^^^^^^^^^^^^^ Element. Property not found in
                              v-
234:   declare export default {|
235:     +DOM: typeof DOM,
236:     +PropTypes: typeof PropTypes,
...:
248:   |};
       -^ object type. See lib: /private/tmp/flow/flowlib_397356f7/react.js:234

Error: node_modules/react-scrollbar-size/node_modules/react-event-listener/src/index.js:102
102: class EventListener extends Component {
                                 ^^^^^^^^^ identifier `Component`. Too few type arguments. Expected at least 1
 29: declare class React$Component<Props, State = void> {
                                   ^^^^^^^^^^^^ See type parameters of definition here. See lib: /private/tmp/flow/flowlib_397356f7/react.js:29

Thank you for your patience here.

rosskevin commented 7 years ago

You are getting the right flow errors with my .flowconfig, because you are using a newer flow-bin than the material-ui package, so you are seeing a new breaking change flow introduced for 0.54.0 if I recall correctly, which is already fixed on the tip of v1-beta.

If you are going to use flow, you are going to have to check https://github.com/facebook/flow/releases for notes on breaking changes/new assertions.

ikzjfr0 commented 7 years ago

i still got bunch of errors. I do use the .flowconfig that you provided above.

Below are my environment: "material-ui": "^1.0.0-beta.9", "react": "^16.0.0-rc.2", "react-dom": "^16.0.0-rc.2", "flow-bin": "^0.53.1", (i also tried 0.54.1)

rosskevin commented 7 years ago

So now that you are on beta 9, which version of flow-bin do you think you should be using (based on the instructions in this issue)?

tugorez commented 7 years ago

Thanks @rosskevin, based on your response I have this .flowconfig working with

[ignore]                                             
.*/node_modules/material-ui/test-utils/.*            
.*/node_modules/jss/lib/renderers/DomRenderer.js.flow

[include]                                            

[libs]                                               
./node_modules/jss/flow-typed

[lints]

[options]
unsafe.enable_getters_and_setters=true               
ikzjfr0 commented 7 years ago

@rosskevin you should add following line into [ignore] part too to make perfect, otherwise, it fails: ./node_modules/react-scrollbar-size/node_modules/react-event-listener/.*

By the way, then this will work on both react 15.6.1 and react 16 rc2

rosskevin commented 7 years ago

@ikzjfr0 this line should have already covered it .*/node_modules/react-event-listener/src, worst case, switch it to .*/node_modules/react-event-listener. It is not a good practice to spell out the path to node_modules, as they can be deeply nested. Always start with .*/node_modules

zachwolf commented 6 years ago

@rosskevin you mentioned having it working in a repo, is there anyway could share that code?

I know supporting flow isn't the purpose of this forum, but I've exhausted other support places and I'm still not having luck consuming the react MUI flow types.

rosskevin commented 6 years ago

Here is a from scratch setup:

https://github.com/rosskevin/flow-example-material-ui

@oliviertassinari - do we want to document, or add this to the create react app, or simply point to this repo?

oliviertassinari commented 6 years ago

@rosskevin A third example in the /examples folder would be perfect :). /with-flow for instance.

rosskevin commented 6 years ago

PR submitted. I'll leave my repo up as a canary for any future problems. I hooked up CircleCI on it.

oliviertassinari commented 6 years ago

@rosskevin In this case, maybe we can only add a link toward your repository in the README of the third example folder? What do you prefer?

rosskevin commented 6 years ago

I'm not sure, but I think people are more likely to look direct at the examples folder here. Let's merge the PR and see how it goes.

zachwolf commented 6 years ago

@rosskevin this is awesome, thank you! 🎉

Using your code as an example, I'm thinking I was expecting Flow to catch some things that it's not built to. For example, changing the <Button /> prop to something invalid passes. Like:

- <Button raised color="accent" onClick={this.handleClick}>
+ <Button raised color="foo" onClick={this.handleClick}>
screen shot 2017-09-29 at 12 43 33 pm
$ flow
No errors!

As a sanity check to make sure Flow is running correctly, I wrote a function in the same index.js file and intentionally pass a wrong param. Flow does catch this error:

screen shot 2017-09-29 at 12 43 17 pm
$ flow
Error: src/pages/index.js:36
 36: foo('c')
         ^^^ string. This type is incompatible with the expected param type of
 32: function foo (param?: 'a' | 'b') {
                           ^^^^^^^^^ string enum

Found 1 error

Also, if I edit flow-example-material-ui-master/node_modules/material-ui/Button.js.flow and add something like this:

Button.defaultProps = {
  // deleted for brevity... 
};

+ function Foo (props: {}) {
+   return (
+       <Button color="test">
+         test
+       </Button>
+   )
+ }

export default withStyles(styles, { name: 'MuiButton' })(Button);

and run Flow on that file directly, it catches the wrong prop:

screen shot 2017-09-29 at 2 15 33 pm
$ flow
Error: node_modules/material-ui/Button/Button.js.flow:282
282:       <Button color="test">
           ^^^^^^^^^^^^^^^^^^^^^ React element `Button`
223: function Button(props: DefaultProps & Props) {
                            ^^^^^^^^^^^^ property `classes`. Property not found in
282:       <Button color="test">
           ^^^^^^^^^^^^^^^^^^^^^ React element `Button`

Error: node_modules/material-ui/Button/Button.js.flow:282
282:       <Button color="test">
           ^^^^^^^^^^^^^^^^^^^^^ React element `Button`
282:       <Button color="test">
                         ^^^^^^ string. Expected string literal `default`, got `test` instead
154:   color: 'default',
              ^^^^^^^^^ string literal `default`

Found 2 errors

My assumption was these errors would show up in my codebase as well

So, to summarize my questions,

Thanks so much for spending time on this issue

rosskevin commented 6 years ago

@zachwolf you are using beta.12. This is fixed in https://github.com/callemall/material-ui/pull/8419

Read it for an explanation.

wcandillon commented 6 years ago

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.