add examples with Flowtype support + libdefs for redux and react-redux #1887
Conversation
|
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:
But 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 😄), so adding code that no one can really own isn't going to serve us well. |
|
I would like to have a Flow example.
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:
|
|
Well, my bad then. Sorry for closing! |
|
@gaearon |
|
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 |
@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 I'd be happy with either the |
|
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 The pattern I've used is each reducer file exports it's own state + I've never been able to get typing working for the connect HOC though so On 11 August 2016 at 06:24, Giulio Canti notifications@github.com wrote:
|
|
|
||
| declare type Store<S, A: { type: $Subtype<string> }> = { | ||
| // rewrite MiddlewareAPI members in order to get nicer error messages (intersections produce long messages) | ||
| dispatch: Dispatch<S, A>; |
There was a problem hiding this comment.
Wouldn't Dispatch<A> do the job as well?
There was a problem hiding this comment.
Ah yes, that S was an experiment to retain type info in favour of react-redux (I mean the dispatch function injected by connect in some cases) but I guess is unnecessary. Better to discard it and simplify both libdefs, thanks for pointing out.
Should I give the feedback in this PR? |
|
@gcanti I have one problem with your definition, so basically when you have this:
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 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 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: |
|
@kasperpeulen type parameter
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)
Seems an easy fix const connector: Connector<{style: Object}, Props> = connect(
({Counter}: State, {style}) => {
return {...Counter, style}
}
)
Yeah, alas there are (5 overloadings of |
Nice 👍
I see, I tried a bit as well, but I also failed getting it to work like I wanted.
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. That seems to work as well. In this case this is some sense better, as it seems natural to make a style property optional. |
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.
|
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: agentcooper/react-redux-flow-example@0d374f7#diff-08a5a32bd874f1452a71be13b25fa610L30. |
|
|
||
| */ | ||
|
|
||
| declare type Dispatch<S, A> = (action: A) => A; |
There was a problem hiding this comment.
I think this definition is a bit more rigid than the middleware API allows. For example, redux-promise would allow a Dispatch object to take a param action of type {type: T, payload: Promise<P>}, but would then return Promise<{type: T, payload: P}>.
There was a problem hiding this comment.
Good point. Is there a better option than returning any?
Note. The typescript definition file should be modified as well
There was a problem hiding this comment.
I can't think of anything better at the moment, since any middleware could modify this in all sorts of ways. It might be possible to pass through the definition of Dispatch from StoreEnhancer. However, I think I remember reading that a reason Flow typedefs for Redux were put on hold last year was because this was difficult to achieve.
|
How would one augment |
|
@kasperpeulen @MarcoPolo @jimbolla Thanks for your comments, I'm trying a new approach using the 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 Could you please try / review my last commit? |
| declare type Null = null | void; | ||
|
|
||
| declare function connect<A, OP>( | ||
| ...rest: Array<void> |
There was a problem hiding this comment.
What is this for? Is it necessary to ensure that the other clauses are used if any params are passed?
There was a problem hiding this comment.
It's a workaround for facebook/flow#2360
There was a problem hiding this comment.
Huh. I had to remove them from redux-saga as they caused issues. I wonder if I introduced more issues... also is a better workaround to reorder them? Maybe they're the same...
There was a problem hiding this comment.
I would add a comment about why it's necessary to do this, with a link to the issue.
There was a problem hiding this comment.
@aaronjensen moving it to the last position doesn't work, you get "Case 4 may work: But if it doesn't, case 6 looks promising too: Please provide additional annotation(s) to determine whether case 4 works"
@jimbolla done
|
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 flow-typed/flow-typed#318 |
|
I'm presently trying to grok all of this information. So, as of right NOW, there's a import type { Dispatch as ReduxDispatch } from "redux";BUT, as of flow-typed/flow-typed#318, when it gets merged, the AMIRITE?! Loving the work you guys are all doing! |
|
type safe |
|
Awesome. I'd say we should incorporate it. |
|
@aaronjensen in my last version (sent to flow-typed) I also improved declare type ActionCreator<A, B> = (...args: Array<B>) => A;
declare type ActionCreators<K, A> = { [key: K]: ActionCreator<A, *> };
declare function bindActionCreators<A, C: ActionCreator<A, *>>(actionCreator: C, dispatch: Dispatch<A>): C;
declare function bindActionCreators<A, K, C: ActionCreators<K, A>>(actionCreators: C, dispatch: Dispatch<A>): C;
What about users of Flow 0.32-? As @jcreamer898 pointed out, it's not yet clear to me how to handle these things properly |
|
@gcanti flow-typed allows different versions for a lib via subdir. For example, redux currently has a Any libs utilizing Where did you send your updated definitions to? I can't find any related PRs. |
|
@rsolomon I know, I was talking about redux which at the moment contains a copy of the definition files The PR is here flow-typed/flow-typed#318 |
|
@gcanti did you ever attempt to use this with react-redux@5.0.x? |
|
@mull no, I didn't |
|
@ccorcos did you ever come to a solution to the problem mentioned above: If I understood you correctly Prop validation on a component isn't occurring when wrapped with connect. Particularly when it is getting some of it's props from it's parent. Which is what i'm seeing. In my case my connect is partially applied so it can take different react component's. Which I can't see a good example for in this PR |
|
I'm having the same problem as @azaharakis, did anyone come to a solution? It's worrisome that I lose prop validation as soon as I connect my component. 🙁 |
|
@AndrewSouthpaw I've been using this for class-based components and it maintains type safety as expected. I've been meaning to submit it for PR to It is largely lifted from the existing definitions there, but it provides much more sound type safety for me. Hopefully it can help you out. |
|
Wow, that's some bonafide magic right there @rsolomon, it works like a charm (I don't use stateless components), thanks for sharing. Did you come up with that yourself? I'm trying to get better at flow typing and maybe contribute to the library, but I get so lost when looking at a file like that one. What was your key change(s)? |
|
Nah I didn't completely come up with it. It's largely based on this definition, but as written that file provided me no type safety. I had used similar strategies for typing |
In particular, avoiding catch-all types like |
reduxjs#1887) * libdefs * todos-flow example * fix react-redux libdef using the `$Supertype` magic type * add comment to the workaround and link to the relevant issue
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-flowtodos-flowtodomvc-flowAdded libdefs (in the
ROOT/flow-typeddirectory)reduxreact-reduxObservations
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 safecombineReducersmay be replaced by combining the reducers by hand (see the todos-flow example).bindActionCreatorsis unsafe when an object is passed in.HoweverbindActionCreatorsalso accepts a single function which is safer (see the todomvc-flow example)react-redux's libdef was pretty hard to write. Should cover the most common cases (at least)In order to please Flow I've done some refactorings but I tried to keep them as small as possible
libdefs need community feedback, then we might consider to push them to flow-typed