Flow 0.53 prop types

[pascalduez]

Hi,

with the latest Flow@0.53 the Type detection for prop types is broken.

Before

import React from 'react';

type Props = {
  foo: string,
};

class Bar extends React.Component {
  props: Props;
}

After

import * as React from 'react';

type Props = {
  foo: string,
};

class Bar extends React.Component<Props> {}

'foo' is missing in props validation (react/prop-type)

[jseminck]

We currently have test cases like this. Are these still valid with Flow 0.53?

type Person = {
  firstname: string
};
class Hello extends React.Component<void, { person: Person }, void> {
  render () {
    return <div>Hello {this.props.person.lastname}</div>;
  }
}

[ljharb]

Why import * as React from 'react'? There's no need to do that; import React from 'react'; is correct.

[EvHaus]

@ljharb I was wondering about that too. However, Flow's official documentation says this change to the import is required and their codemods make this change as well (see https://github.com/facebook/flow/blob/a4df443a4a4fb5f62b86f03ee8a12a2b1432296a/website/en/docs/react/types.md).

Not sure if we can get someone from the Flow team to chime in, but it seems totally unnecessary to me as well (and potentially troublesome when using Webpack's Tree Shaking).

[ljharb]

That's horrific; it's one of the least usable parts of TypeScript.

[ljharb]

and yes, it also makes tree-shaking impossible unless you can statically determine every key accessed on a ModuleRecord.

[pascalduez]

Yep, this might not be needed for the dummy example provided above, but will be as soon as you need one of the exposed types.
And yes the codemod convert all those imports anyway.

https://github.com/facebook/flow/blob/v0.53/Changelog.md#0530

[benjaminRomano]

I assume they are using the import * as ... syntax because they want people to use their types prefixed with React.

For example,

import * as React from 'react';

let element: React.Node = ...;

is equivalent to

import React from 'react';
import type {Node} from 'react';

let element: Node = ...;

However, the first example is "cleaner".

[ljharb]

If the types are in the react package, then they should be importable directly. If they're not normal named imports, import * shouldn't be able to bring them in, per spec.

[tgallacher]

This is also a problem I have come across after upgrading to Flow@0.53 with `eslint-plugin-react@7.2.1.

I think Flow wants you explicitly use Reacts utility types, and types are not automatically attached to the default export as it isn't always guaranteed to be an object -- could be a number, for example -- and so you need to do a full import under a namespace. If you only need a single type, and want to take advantage of tree-shaking you could combine them in one line like:

import React, { type Node } form 'react'

Note, theres a brief outline of this towards the end of the Components section on Flow docs

[ljharb]

The outline makes sense; in that case it's a poor choice for a codemod unless it's super smart about when it uses *.

[tgallacher]

Temp fix appears to be to use both the new required generics approach enforced by Flow@0.53, and the (now) legacy approach. For example:

import React, { type Element } from 'react'

type Props = {
   exA: number
};

type State = {
   exB: string
};


// 
// Latest version of Flow wants you to use generics
// but current version of Eslint plugin warns against react/prop-types
class Foo extends React.Component<Props, State>{
    props: Props; // This will keep "eslint-plugin-react" happy without interfering with Flow new generics req.

    state = {
        exB: 'initial val'
    };

    render(): Element<'div'>{
        const { exA } = this.props;
        const { exB } = this.state;

        return (
            <div>
                <ul>
                    <li>Props val: { exA }</li>
                    <li>(initial) State val: { exB }</li>
                </ul>
            </div>
        );
    }
}

[jamiebuilds]

To summarize with the exact reasoning:

import React from 'react'; // `React` is an object
import * as React from 'react'; // `React` is a namespace

Types can exist in namespaces, but not in objects.

You can also import specific bindings for both JavaScript values and Flow types:

import { Component } from 'react';
import type { Node } from 'react';
import { type Node } from 'react';

And combine them if you want to import both the object and a type:

import React, { type Node } from 'react';

[jamiebuilds]

Because the type parameter signature changes in Flow 0.53 from:

React.Component<DefaultProps, Props, State>

to:

React.Component<Props>
// or 
React.Component<Props, State>

This ESlint plugin could be updated to pull props from:

React.Component { props: Props; }
React.Component<X, Props, X> {}
React.Component<Props, X> {}
React.Component<Props> {}

And it will support all versions of Flow

[jseminck]

@thejameskyle in flow <0.53, was this also valid syntax: React.Component<X, Props> {} ? (2 arguments, where Props is the second argument)

If so, then we cannot distinguish properly between the old and the new syntax. If we expect Props to be the first or the second TypedArgument, then we'll be considering State as Props for those users who use the new React.Component<Props, State> {} syntax, right?

Ofcourse we could do it so that if there are 3 arguments we check the second argument, and if there are 2 or 1 then we check the first. I just assumed that people may already have code like React.Component<X, Props> {} with the older Flow and then that would break (because we would only check the first argument as there are only 2 arguments in total).

Edit: We could also enforce the name to be Props, but the current implementation allows the name to be anything or even an object directly (as has been requested by some users):

• https://github.com/yannickcr/eslint-plugin-react/blob/master/tests/lib/rules/prop-types.js#L2862
• https://github.com/yannickcr/eslint-plugin-react/blob/master/tests/lib/rules/prop-types.js#L2918

The current idea we discussed in the PR is to introduce a flowVersion configuration, similar to how we already have a version configuration for React.

[jamiebuilds]

No, in previous versions of Flow, you could only have no args or three args:

// Flow <0.53
class extends React.Component {}
class extends React.Component<DefaultProps, Props, State> {}

class extends React.Component { props: Props; }
class extends React.Component<DefaultProps, Props, State> { props: Props; }

// Flow >=0.53
class extends React.Component {}
class extends React.Component<Props> {}
class extends React.Component<Props, State> {}

class extends React.Component { props: Props; }
class extends React.Component<Props> { props: Props; }
class extends React.Component<Props, State> { props: Props; }

[jseminck]

Great. I think we can use that approach then! 👍

3 arguments: check the second argument.
2 or 1 argument: check the first argument.

Thanks for your input. 😄

[oliviertassinari]

Why import * as React from 'react'?

One important limitation of this change is around React.propTypes and React.createClass that are accessed. With react v15.x, it's raising two warnings:

Explicitly importing the types is more appropriate until React v16 is released as stable.

[ljharb]

@oliviertassinari this is because you're supposed to use the prop-types and create-react-class packages as of v15.5, which work all the way back to 0.13 - so React.PropTypes isn't ever appropriate anymore anyways.

[oliviertassinari]

@ljharb It's a different topic. This change introduces the warnings with the @latest version of react:

-import React from 'react';
+import * as React from 'react';

And it's fixed with the @next version of react

[ljharb]

ohhh i see what you mean, gotcha. that seems like a reason to use import React from 'react' instead anyways.

[jseminck]

This is now in master. It would be great if someone could try on their codebase(s) and confirm everything works!

[johnhaley81]

@jseminck I pulled in https://github.com/yannickcr/eslint-plugin-react#e2f6460a63bd0cd0dca0c8308e35ff831d5161ed but I'm still not getting prop validation detection with flow types. I also added:

"settings": {
    "react": {
      "flowVersion": "0.53"
    }
  }

to my .eslintrc.js file. Is there something else I'm missing?

[jseminck]

Thanks for testing it, @johnhaley81. Appreciate that! 🎉 Can you confirm you are using the master release? I did this to get it:

rm -rf node_modules/eslint-plugin-react
yarn add https://github.com/yannickcr/eslint-plugin-react -D

One way to test it is to execute this command: cat node_modules/eslint-plugin-react/lib/rules/prop-types.js | grep versionUtil and it should return:

const versionUtil = require('../util/version');
propsParameterPosition = versionUtil.testFlowVersion(context, '0.53.0') ? 0 : 1;

You shouldn't have to add the flowVersion to the config yet, though it doesn't hurt for the future. It should definitely work without.

Here is my example which does seem to work. bar is recognised, but foo is not. If you still have unexpected output then please provide a snippet and I'll look into it. I realise now that it is time for me to start using Flow in some side projects. 😄

[alexreardon]

Thanks for working on this everyone - it now works great!