react-powerplug icon indicating copy to clipboard operation
react-powerplug copied to clipboard

Export flow type definitions

Open romandecker opened this issue 7 years ago • 6 comments

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?

romandecker avatar Aug 16 '18 08:08 romandecker

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

TrySound avatar Aug 16 '18 08:08 TrySound

It's actually just because of the linter rule

romandecker avatar Aug 16 '18 09:08 romandecker

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.

TrySound avatar Aug 16 '18 11:08 TrySound

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 :smile:, 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?

romandecker avatar Aug 16 '18 11:08 romandecker

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.

TrySound avatar Aug 16 '18 12:08 TrySound

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.

romandecker avatar Aug 17 '18 09:08 romandecker