Closed gcanti closed 7 years ago
I don't think we need the examples to be run with Flow. We don't have any other examples that use TypeScript, by comparison.
I do think there is value in the Flow definitions, though. Can you open that up as a separate PR?
Edit: I DO think there's value in the defs! Whoops!
@timdorr I added the examples (and I do believe they are very important) for several reasons:
.flowconfig
real world examples (e.g. how to handle import 'todomvc-app-css/index.css'
in todomvc)combineReducers
or bindActionCreators
, may be less useful or even unsafeBut even more important, while libdefs alone seem the main asset, examples are crucial for beginners: adding type annotations (but I'd say more in general adding type safety) is not always a straightforward process. Moreover, these examples are valuable because show the transition from an untyped codebase to a typed one.
We test our TypeScript defs. Is there no equivalent testing infrastructure for Flow?
All of what you're saying describes Flow-specific issues and idioms. Those could apply to any other library, not just Redux. I don't want us to take on a language extension that will be unfamiliar to beginners or indicate some pattern they should be following. In fact, this is what Dan is trying to do in #1883, where create-react-app will let us get rid of a lot of the tooling infrastructure that distracts from the important part: the code.
Also, none of regular contributors are Flow experts (otherwise, we would have done this a year ago when Dan brought it up :smile:), so adding code that no one can really own isn't going to serve us well.
I would like to have a Flow example.
Not for every single example, but having something like todos-flow
is something I want.
However it would need to be rebased on top of #1883 and use the same infra.
In fact, this is what Dan is trying to do in #1883, where create-react-app will let us get rid of a lot of the tooling infrastructure that distracts from the important part: the code.
I want Flow to eventually be included by default with Create React App. We might also start using it more in React docs as it becomes better suited to common workflows / stacks in the community.
I would like to reopen and re-scope this in the following way:
todos-flow
example.flow-typed
folder works: does this mean we’ll ship Flow definitions? Or would users have to copy-paste it? I want todos-flow
to work out of the box if possible. If we need to ship definitions, I’d like to do that.Well, my bad then. Sorry for closing!
@gaearon flow-typed
folder within a project is automatically used by flow as a libdef folder. To make it work out of the box the definitions should be shipped (Draft.js team has made some progress on that as far as I know — they ship their definitions with the lib). Another possibility is to add it to (flow-typed)[https://github.com/flowtype/flow-typed/] repo. There is a community created libdef for redux there, but having an official one is always better.
If someone is willing to lead the effort of official Flow types I’m happy to assist in any way I can.
@gaearon I guess we just need to decide between flow-typed repo or shipping with redux
I guess we just need to decide between flow-typed repo or shipping with redux
@thejameskyle Any opinions on this?
@gaearon if there's something I can do I'm glad to help. I worked on this for a few weeks, when you find the right tradeoff, flow is awesome
@gcanti Do you mind splitting out the single commit into one for each example and the libdefs separately? Even better would be a separate PR for the definitions and the examples, but you don't have to do that. Reading through 2,675 new lines makes my browser crawl 😄
Thank you so much! 💯
So, I'm with @gaearon on keeping this to just one single example. Duplicating examples means pain whenever one is updated and you have to synchronize the changes to the other. 3 examples mean that happens 3x as often.
Given the size of the todomvc-flow
example (1,560 lines of code), that one should go. I know it has some things to help with dealing with CSS imports, but that's a Flow issue, not a Redux one. So, I don't think we're in any position here to be establishing patterns for another library.
I'd be happy with either the todos-flow
or counter-flow
examples being added. It looks like todos-flow
has more substantial Flow-specific additions (namely, a separate types definition for the app), so it seems like the better choice. What are your thoughts, @gcanti?
I agree, one single example is enough. counter-flow is too simple while todomvc-flow is unnecessary big.
todos-flow seems perfect, is fairly small but contains all the juice. It shows how to:
The other thing that I've been doing for this is typed check tests to go with it.
The pattern I've used is each reducer file exports it's own state + initialState and each corresponding test file exports some populated typed test data. That way in my root reducer I can combine states and in my root reducer test file I have access to typed test state from other reducers so I can test the selectors that select across more than one reducer.
I've never been able to get typing working for the connect HOC though so
I've been getting the mapStateToProps to export StateProps and
mapDispatchToProps to export DispatchProps.
Then my props type for the component is type Props = StateProps & DispatchProps & IncomingProps;
. This works but feels super hacky.
On 11 August 2016 at 06:24, Giulio Canti notifications@github.com wrote:
I agree, one single example is enough. counter-flow is too simple while todomvc-flow is unnecessary big.
todos-flow seems perfect, is fairly small but contains all the juice. It shows how to:
- configure a minimal .flowconfig file
- type check the action creators
- type check the reducers
- write a type safe combined reducer
- type check stateless components
- type check the containers and connect them via react-redux with the connector pattern
- group the domain model in a separate file
- tie all together by annotating the store
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/reactjs/redux/pull/1887#issuecomment-238992269, or mute the thread https://github.com/notifications/unsubscribe-auth/ACA_f6R9kObU267BOfxtX5UR_jjM1h-1ks5qejOEgaJpZM4JgNAn .
4) libdefs need community feedback, then we might consider to push them to flow-typed
Should I give the feedback in this PR?
@gcanti I have one problem with your definition, so basically when you have this:
const connector: Connector<OwnProps,Props> = connect(...)
It now works like that Props are the Props of the component. And OwnProps are the Props that the connected Component exposes. It assumes that Props = SP & { dispatch: Dispatch<S, A> }
where SP
are the props it gets from the redux State. But this is not true right? The props can also come from the view, it doesn't have to come from the redux state.
I get a bunch of errors of code that work, because of that (verryyy looongg error messages). But I think this is the reason.
For example, say I have:
<Counter style={{color: 'red'}}/>
where Counter is
type Props = {
show: boolean,
value: number,
dispatch: Dispatch<State, Action>,
style: Object
}
const Counter = ({show, value, dispatch, style}: Props) =
Now show
and value
is coming from the redux state like this:
const connector: Connector<{style: Object}, Props> = connect(
({Counter}: State) => {
return {...Counter}
}
);
Where Counter has type
{
value: number,
show: boolean
}
So everything works if I run it, but I get the following errors:
src/Counter/index.js:0
inconsistent use of library definitions
33: declare type Connector<OP, P> = {
^ intersection. This type is incompatible with. See lib: flow-typed/react-redux.js:33
34: (component: StatelessComponent<P>): ConnectedComponentClass<OP, P, void, void>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See lib: flow-typed/react-redux.js:34
Member 1:
34: (component: StatelessComponent<P>): ConnectedComponentClass<OP, P, void, void>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See lib: flow-typed/react-redux.js:34
Error:
38: const connector: Connector<{style: Object}, Props> = connect(
^^^^^ object type. This type is incompatible with. See: src/Counter/Counter.js:38
52: ): Connector<OP, SP & { dispatch: Dispatch<S, A> }>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ intersection. See lib: flow-typed/react-redux.js:52
Member 1:
38: const connector: Connector<{style: Object}, Props> = connect(
^ type parameter `SP` of function call. See: src/Counter/Counter.js:38
Error:
38: const connector: Connector<{style: Object}, Props> = connect(
^^^^^ property `style`. Property not found in. See: src/Counter/Counter.js:38
40: return {...Counter}
^^^^^^^^^^^^ object literal. See: src/Counter/Counter.js:40
Member 2:
52: ): Connector<OP, SP & { dispatch: Dispatch<S, A> }>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object type. See lib: flow-typed/react-redux.js:52
Error:
38: const connector: Connector<{style: Object}, Props> = connect(
^^^^^ property `style`. Property not found in. See: src/Counter/Counter.js:38
52: ): Connector<OP, SP & { dispatch: Dispatch<S, A> }>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object type. See lib: flow-typed/react-redux.js:52
Member 2:
35: <Def, St>(component: Class<React$Component<Def, P, St>>): ConnectedComponentClass<OP, P, Def, St>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ polymorphic type: function type. See lib: flow-typed/react-redux.js:35
Error:
inconsistent use of library definitions
21: declare type StatelessComponent<P> = (props: P) => ?React$Element<any>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ statics of function type. Callable signature not found in. See lib: flow-typed/react-redux.js:21
16: declare class React$Component<DefaultProps, Props, State> {
^ statics of React$Component. See lib: /private/tmp/flow/flowlib_377a32d6/react.js:16
src/Counter/index.js:0
inconsistent use of library definitions
33: declare type Connector<OP, P> = {
^ intersection. This type is incompatible with. See lib: flow-typed/react-redux.js:33
35: <Def, St>(component: Class<React$Component<Def, P, St>>): ConnectedComponentClass<OP, P, Def, St>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ polymorphic type: function type. See lib: flow-typed/react-redux.js:35
Member 1:
34: (component: StatelessComponent<P>): ConnectedComponentClass<OP, P, void, void>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See lib: flow-typed/react-redux.js:34
Error:
inconsistent use of library definitions
34: (component: StatelessComponent<P>): ConnectedComponentClass<OP, P, void, void>;
^^^^^^^^^^^^^^^^^^^^^ function type. Callable signature not found in. See lib: flow-typed/react-redux.js:34
35: <Def, St>(component: Class<React$Component<Def, P, St>>): ConnectedComponentClass<OP, P, Def, St>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^ statics of React$Component. See lib: flow-typed/react-redux.js:35
Member 2:
35: <Def, St>(component: Class<React$Component<Def, P, St>>): ConnectedComponentClass<OP, P, Def, St>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ polymorphic type: function type. See lib: flow-typed/react-redux.js:35
Error:
38: const connector: Connector<{style: Object}, Props> = connect(
^^^^^ object type. This type is incompatible with. See: src/Counter/Counter.js:38
52: ): Connector<OP, SP & { dispatch: Dispatch<S, A> }>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ intersection. See lib: flow-typed/react-redux.js:52
Member 1:
38: const connector: Connector<{style: Object}, Props> = connect(
^ type parameter `SP` of function call. See: src/Counter/Counter.js:38
Error:
38: const connector: Connector<{style: Object}, Props> = connect(
^^^^^ property `style`. Property not found in. See: src/Counter/Counter.js:38
40: return {...Counter}
^^^^^^^^^^^^ object literal. See: src/Counter/Counter.js:40
Member 2:
52: ): Connector<OP, SP & { dispatch: Dispatch<S, A> }>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object type. See lib: flow-typed/react-redux.js:52
Error:
38: const connector: Connector<{style: Object}, Props> = connect(
^^^^^ property `style`. Property not found in. See: src/Counter/Counter.js:38
52: ): Connector<OP, SP & { dispatch: Dispatch<S, A> }>;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object type. See lib: flow-typed/react-redux.js:52
Found 2 errors
@kasperpeulen type parameter S
removed from Dispatch
.
It assumes that Props = SP & { dispatch: Dispatch<S, A> } where SP are the props it gets from the redux State. But this is not true right?
Yes, theorically you are right. Technically, in order to type check as much as possible, I need that property (I tried for hours other approaches but I failed. I'm open to alternative implementation though)
I get a bunch of errors of code that work
Seems an easy fix
const connector: Connector<{style: Object}, Props> = connect(
({Counter}: State, {style}) => {
return {...Counter, style}
}
)
verryyy looongg error messages
Yeah, alas there are (5 overloadings of connect
) x (2 ways of defining a component), i.e. stateless and React$Component
, thus the error messages are long and awkward
type parameter S removed from Dispatch.
Nice 👍
Yes, theorically you are right. Technically, in order to type check as much as possible, I need that property (I tried for hours other approaches but I failed. I'm open to alternative implementation though)
I see, I tried a bit as well, but I also failed getting it to work like I wanted.
Seems an easy fix
That works indeed. Strangely enough, this did not work:
const connector: Connector<{style: Object}, Props> = connect(
({Counter}: State, ownProps) => {
return {...Counter, ...ownProps}
}
)
If this will be chosen as the way to go, I think it is important to have good documentation.
type Props = {
show: boolean,
value: number,
dispatch: Dispatch<State, Action>,
style?: Object
}
That seems to work as well. In this case this is some sense better, as it seems natural to make a style property optional.
Strangely enough, this did not work
Yeah, kind of surprising. The spread works if you add a type annotation
type OwnProps = {
style: Object
};
const connector: Connector<OwnProps, Props> = connect(
({Counter}: State, ownProps: OwnProps) => {
return {...Counter, ...ownProps}
}
)
@gcanti Thank you for working on this, proper definitions for react-redux are the missing piece for full Flow coverage on our project.
I'm trying to compile a simple example with your definitions: https://github.com/agentcooper/react-redux-flow-example.
App
renders Counter
without text
prop and Flow does not seem to pick up the error. Is this solvable?
@agentcooper
flow-typed
dir to the project root (otherwise flow doesn't see it)// @flow
pragma to types/index.js
you'll get a few errors
@gcanti just did that and found out that I have the same issue as @kasperpeulen. It is actually working now, however merging ownProps with stateProps looks little bit awkward: https://github.com/agentcooper/react-redux-flow-example/commit/0d374f7477a56758bff9aa2ef0048ee5a6ff0e6a#diff-08a5a32bd874f1452a71be13b25fa610L30.
How would one augment Dispatch<A>
with the thunk middleware? You can't just include a thunk in A
because it doesn't have a type
property. Would you have to edit the types directly to do this or is there another way?
@aaronjensen take a look at https://github.com/agentcooper/react-redux-flow-example/blob/master/src/types/index.js#L20
@agentcooper thanks, that worked. I didn't know you could intersect functions like that.
@gcanti I've been playing around with Flow and Redux and this is by far the best example yet. 🏆🏆🏆
In some of the other implementations I've seen, I'm not sure they completely understand the concept of "strongly typed". Even the flow-typed definitions are pretty crappy if you ask me -- state is annotated as an Object!
Anyways, I had some feedback to give you, but I answered all my own questions as I went back through to try find the answers. This looks awesome. I'm going to play with it today. Let me know if I can help.
Also, it would be great if these lib defs were in .js.flow
format so we dont have mess with the flowconfig file
I've gotten this to work pretty well for me. The only thing that isnt working is validating component props after connect
.
For example:
// @flow
import { connect } from 'react-redux'
import React from 'react'
import classnames from 'classnames'
import UnderlineSrc from 'skeleton/src/images/underline.png'
import 'skeleton/src/components/counter/style.css'
import * as actions from 'skeleton/src/actions'
import type { State, Dispatch, Action } from 'skeleton/src/types'
type Props = {
color: 'red' | 'blue',
pending: boolean,
count: number,
decrement: Function,
increment: Function,
}
const Counter = (props : Props) => {
const className = classnames({
counter: true,
disabled: props.pending,
})
return (
<div className={className} style={{color: props.color}}>
<div>
<button className="decrement" onClick={props.decrement}>{'-'}</button>
<span className="count">{props.count}</span>
<button className="increment" onClick={props.increment}>{'+'}</button>
</div>
<img alt="underline" className="underline" src={UnderlineSrc} />
</div>
)
}
const mapStateToProps = (state: State) => ({
count: state.get('count'),
pending: state.get('pending').includes('decrement'),
})
const mapDispatchToProps = (dispatch: Dispatch) => ({
increment: () => dispatch(actions.increment()),
decrement: (count) => () => dispatch(actions.decrement(count)),
})
const mergeProps = (stateProps, dispatchProps, ownProps) => ({
...stateProps,
...dispatchProps,
...ownProps,
decrement: dispatchProps.decrement(stateProps.count),
})
export default connect(mapStateToProps, mapDispatchToProps, mergeProps)(Counter)
And this doesnt throw an error:
ReactDOM.render((
<Provider store={store}>
<Counter/>
</Provider>
), root)
@ccorcos try this pattern https://github.com/reactjs/redux/pull/1887/files#diff-ef2fbfc16613d42f52b86d75581efc9aR27
hmm. @gcanti don't you think its possible somehow to deduce OwnProps?
I'm toying around with little success. I was thinking something like this:
declare type Connect<State,Action,Props> = (
mapStateToProps: (state: State, ownProps: $Diff<Props,ResultProps>) => StateProps,
mapDispatchToProps: (dispatch: Dispatch<Action>, ownProps: $Diff<Props,ResultProps>) => DispatchProps,
mergeProps: (stateProps: StateProps, dispatchProps: DispatchProps, ownProps: $Diff<Props,ResultProps>) => ResultProps
) => Connector<$Diff<Props,ResultProps>, Props>
Even in your example, you need to specify State and Dispatch in mapStateToProps and mapDispatchToProps. That doesnt seem like it should be necessary...
So even when I do this:
const connector : Connector<{color: 'red' | 'blue'}, Props>= connect(mapStateToProps, mapDispatchToProps, mergeProps)
export default connect(Counter)
This checks out fine:
ReactDOM.render((
<Provider store={store}>
<Counter/>
</Provider>
), root)
@ccorcos please put your code in a repo, it's hard to tell why from a fragment
@gcanti I put it all here: https://github.com/gcanti/redux/pull/1
@gcanti @gaearon what are we waiting for to merge this?
I've been following along with this thread (this is our biggest open PR by number of comments), but have been waiting for the dust to settle before approaching it. I'm not a Flow expert (I'm not even a typings expert), so I'm reliant on the comments from others to determine if this should be merged in or not.
I think I just need a consensus at this point. So, any participants or onlookers want to comment on the suitability of these typings currently?
I also need remove the todomvc-flow and counter-flow examples, as discussed before, and update todos-flow to use Create React App (if possible?). But those things can happen separately right after the merge process.
So
todos-flow
example1, 2 seem easy, I'm not sure what to do for 3 though. Once we agree on 3, I can close this PR and open a new one meant to be merged. @gaearon @thejameskyle any advices?
I think I'd lean towards flow-typed only because they have a testing infrastructure in place and building that out here looks like a lot of work, which would be a burden on those contributing who aren't concerned with Flow-related code.
@gcanti Can you git rebase -i master
and drop the todosmvc-flow and counter-flow commits? I think only cc0b9ae8 might give you some trouble. If you don't want to wrestle with git stuff, let me know. I can take that on after-the-fact once we get things merged in appropriately.
And good job everyone for the comments on and subsequent fixes to the definitions!
@timdorr rebased and squashed (2 commits now):
This is looking great! I'm curious what @thejameskyle thinks about whether these typedefs should be here or in flow-typed.
@ccorcos my understanding is that until shadow files are a fleshed out thing, flow-typed is the place they're meant to be.
Playing around with the typedefs. Quite amazing. I'm not sure how would I go about typing my own middleware if I want it to be generic. Another thing (which is probably not possible with flow, at least yet) — if my app is structured in this way:
module1/
component.js
container.js
reducer.js
actions.js
types.js
index.js
module2/
...
I can import my actions and have a union type type Action = ModuleOneAction | ModuleTwoAction
so my actions file is decoupled from the app. But container and reducer are still coupled because the reducer will be called with the app Action
type and the container uses Dispatch that in its turn uses ReduxDispatch<Action>
as well as the Store
type.
I wonder what one would do if his module depends only on its own Actions and Store property.
``
@kasperpeulen @MarcoPolo @jimbolla Thanks for your comments, I'm trying a new approach using the $Supertype
magic type:
declare function connect<S, A, OP, SP>(
mapStateToProps: MapStateToProps<S, OP, SP>,
mapDispatchToProps: Null,
mergeProps: Null,
options?: ConnectOptions
): Connector<
OP,
$Supertype< // <= this should allow the `& OP` intersection below and "optional" field declarations in the connected component (@MarcoPolo)
SP &
{ dispatch: Dispatch<A> } &
OP> // <= this should account for the default merge function (@kasperpeulen and @MarcoPolo)
>;
Also the wrong connect
overloading should be fixed (@jimbolla comment)
Could you please try / review my last commit?
OK, we've got @MarcoPolo, @aaronjensen, and @alexeygolev all saying this looks good. So, it's on you 3 if the community gets out their pitchforks 🔥 😈
But seriously, thanks for the awesome work on this @gcanti and the help from everyone who commented. Couldn't have done it without you all!
@gcanti what is the best way to consume this today. I have the redux and react-redux libdef from flow-typed installed right now. Are you going to publish there? Or will this be distributed as part of the npm dep on install and we should point flow to it?
@gnoff they're pull requested https://github.com/flowtype/flow-typed/pull/318
I'm presently trying to grok all of this information. So, as of right NOW, there's a flow-typed
directory that automatically allows me to do this...
import type { Dispatch as ReduxDispatch } from "redux";
BUT, as of https://github.com/flowtype/flow-typed/pull/318, when it gets merged, the flow-typed
repo will contain the declarations, so they'll at that point be pulled from there?
AMIRITE?!
Loving the work you guys are all doing!
type safe combineReducers
using the new $ObjMap
magic type (Flow v0.33+) https://gist.github.com/gcanti/1e4494371c46f7f7ef4fb919778a32e9
Using Flow for statically analyzing state and actions
This PR is a proof of concept and is not meant to be merged
Added examples (ordered by difficulty)
counter-flow
todos-flow
todomvc-flow
Added libdefs (in the
ROOT/flow-typed
directory)redux
react-redux
Observations
1) the best type safety is achieved by avoiding the following helpers
combineReducers
(you can miss a field and / or assign a wrong reducer to a field)bindActionCreators
(all action creator signatures are erased) <= this is now type safecombineReducers
may be replaced by combining the reducers by hand (see the todos-flow example).bindActionCreators
is unsafe when an object is passed in.HoweverbindActionCreators
also accepts a single function which is safer (see the todomvc-flow example)2)
react-redux
's libdef was pretty hard to write. Should cover the most common cases (at least)3) In order to please Flow I've done some refactorings but I tried to keep them as small as possible
4) libdefs need community feedback, then we might consider to push them to flow-typed