Add further support for Flow prop types. #1138
Conversation
This adds support for constructions like this:
type Props = {name: string};
class X extends React.Component<void, Props, void> {
...
}
Which is the preferred way to declare prop types using Flow.
The less preferred syntax is still supported:
class Z extends React.Component {
props: Props;
}
Fixes jsx-eslint#453
|
Why "void, Props, void"? props are the first argument to the constructor; what's the first void for? |
|
@ljharb the first parameter is the default properties, e.g. type Props = {
name: string
};
const defaultProps = {
name: 'Foo'
};
type State = {
foo: string
}
class Y extends React.Component<typeof defaultProps, Props, State> {
state = { foo: 'bar' };
static defaultProps = defaultProps;
// ...
} |
|
can you explain a bit about how Flow handles that syntax in a generic way, such that I'd use it on a non-React component? |
|
@ljharb I guess there are two things here, the type parameters and Type Parameters// So this is a generic container
// `T` can be anything
class Container<T> {
items: T[];
push (...args: T[]) {
this.items.push(...args);
return this;
}
}
// When extending we can pass the type parameter
// to the super class, making the type more specialized
class StringContainer extends Container<string> {
}
const c = new Container();
c.push(true, 123, 'this is fine'); // all good
const sc = new StringContainer();
sc.push('this is fine');
sc.push(false); // flow error$Diff<A, B>It takes two (object) types and returns the properties that exist in one and not the other, hopefully this is helpful: type APIOptions = {
hostname: string,
port: number
};
const defaultOptions = {
port: 80
};
// Note: I don't think $Subtype<> should really be required,
// and it's probably a flow bug. You may be able to omit it.
type RequiredOptions = $Subtype<$Diff<APIOptions, typeof defaultOptions>>;
class API {
opts: APIOptions;
constructor (options: RequiredOptions) {
this.opts = Object.assign({}, defaultOptions, options);
}
getURL () {
return this.opts.hostname + ':' + this.opts.port;
}
}
const api = new API({
hostname: 'foo'
});
console.log(api.getURL()); |
|
@ljharb @yannickcr friendly ping - this is working locally on quite a large codebase so quite confident it works, anything else I need to do before it can be merged? |
lib/rules/prop-types.js
Outdated
| if (seen === void 0) { | ||
| // Keeps track of annotations we've already seen to | ||
| // prevent problems with cyclic types. | ||
| seen = []; |
There was a problem hiding this comment.
we support node 4+ now, so let's use a Set instead of an array here
|
LGTM. We use Flow a lot so I can definitely keep a close eye on this once it gets released to see if it causes any issues, but the test coverage looks good already. |
|
Very excited for this to launch! |
|
Waiting for this to be merged! |
|
Maybe not relevant since #1377 has been merged. |
|
I was not aware of this PR! Yes. It should cover the same functionality as this PR and more ( the new syntax ). This PR contains more test cases though. I will add them as well to make sure they all pass. Edit: I was a bit confused initially. I thought this PR was about Just to clarify: Even though this PR was made before, the support for Flow in It seems all the functionality that this PR added is already supported for prop-types. But for the no-unused-prop-types rule we are still missing some functionalities, but #1412 is the first step to adding that support. |
Originally written by @phpnode as part of jsx-eslint#1138
This adds support for constructions like this:
Which is the preferred way to declare prop types using Flow.
The less preferred syntax is still supported:
Fixes #453, #913