Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export flow type definitions #160

Open
romandecker opened this issue Aug 16, 2018 · 6 comments
Open

Export flow type definitions #160

romandecker opened this issue Aug 16, 2018 · 6 comments

Comments

@romandecker
Copy link

Would it be possible to export all flow type definitions? Here's a use case:

import { Value } from 'react-powerplug';
import { adopt } from 'react-adopt';

const Composed = adopt({
    foo: <Value initial="bar" />,
    baz: <Value initial="qux" />
});

This will make flow complain:

Cannot create Value element because:
 • Either property render is missing in props [1] but exists in object type [2].
 • Or property children is missing in props [1] but exists in object type [3].

 [2] 274│   | {| initial: T, onChange?: ValueChange<T>, render: ValueRender<T> |}
 [3] 275│   | {| initial: T, onChange?: ValueChange<T>, children: ValueRender<T> |}

Which makes sense I guess. This can be fixed by pulling out the render explicitly:

import { Value } from 'react-powerplug';
import { adopt } from 'react-adopt';

const Composed = adopt({
    foo: ({ render }) => <Value initial="bar">{render}</Value>,
    baz: ({ render }) => <Value initial="qux">{render}</Value>,
});

But now, I have to type render (at least with the react/prop-types eslint-rule active).

Sadly, react-powerplug does not currently export the ValueRender type which makes this hard to do... Could you export all flowtypes?

@TrySound
Copy link
Collaborator

@DEX3 Does flow ask you to type this component or only this lint rule?

@romandecker
Copy link
Author

It's actually just because of the linter rule

@TrySound
Copy link
Collaborator

Well, I don't think react/prop-types is a lead rule in flow world. The idea of not exposing types is minimalistic api and type inference. Explicit types doesn't make much sense if adopt has well written type annotation.

If you have more thoughts on this, I will consider them.

@romandecker
Copy link
Author

romandecker commented Aug 16, 2018

You are right that this makes sense from this point of view.

I do however I think that a lot of people might actually be able to profit from the exported types (eslint-plugin-react is pretty popular and the rule is enabled in eslint-config-airbnb for example).

Also one could argue about the types not really increasing the surface area of the API 😄, I mean the typescript types are already being exported, just the flowtypes are missing.

I'm totally up for issuing a PR myself, would you accept the change?

@TrySound
Copy link
Collaborator

TrySound commented Aug 16, 2018

I just don't think prop-types make any sense with flow. These rules should be disabled. TypeScript types are all exported just because of lack of type inference. I was against this idea before.

@romandecker
Copy link
Author

Just to be clear: the eslint-plugin-react-package that provides the react/prop-types rule already supports flow and considers flow-types equivalent to manually using PropTypes. The rule just fails here because there are neither PropTypes, nor flow typings used.

Having a rule that forces you to type your props (independent of whether you use PropTypes or flow) - even if flow could infer them - isn't necessarily bad, as types also serve as a form of documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants