Skip to content

New Take on adding Flow Types #817

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions examples/flow-counter/.babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"stage": 2,
"env": {
"development": {
"plugins": [
"react-transform"
],
"extra": {
"react-transform": [{
"target": "react-transform-hmr",
"imports": ["react"],
"locals": ["module"]
}]
}
}
}
}
11 changes: 11 additions & 0 deletions examples/flow-counter/.flowconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[ignore]
.*/node_modules/

[include]

[libs]
interfaces/


[options]
module.system=node
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can see, I removed the line module.name_mapper='^\(redux\)$' -> '\1/src/index' and opted to continue with Flow declaration modules (the [libs] option) since that regex was conflicting with redux-thunk, and I was not able to create a regex that wouldn't stop that problem. If anyone can give this a try, or discuss approaches, let's start a thread here.

40 changes: 40 additions & 0 deletions examples/flow-counter/actions/counter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/* @flow */

import type { Action, AppState } from '../types';

type Thunk<S, D> = reduxThunk$Thunk<S, D>;

export const INCREMENT_COUNTER = 'INCREMENT_COUNTER';
export const DECREMENT_COUNTER = 'DECREMENT_COUNTER';

export function increment() : Action {
return {
type: INCREMENT_COUNTER
};
}

export function decrement() : Action {
return {
type: DECREMENT_COUNTER
};
}

export function incrementIfOdd() : Thunk<AppState, Action> {
return (dispatch, getState) => {
let { counter } = getState();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this const to a let because of this bug in Flow, that is now fixed in master: facebook/flow#844


if (counter % 2 === 0) {
return;
}

dispatch(increment());
};
}

export function incrementAsync(delay: number = 1000) : Thunk<AppState, Action> {
return dispatch => {
setTimeout(() => {
dispatch(increment());
}, delay);
};
}
32 changes: 32 additions & 0 deletions examples/flow-counter/components/Counter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/* @flow */

import React, { Component } from 'react';

export type CounterProps = {
counter: number,
increment: () => any,
decrement: () => any,
incrementIfOdd: () => any,
incrementAsync: () => any,
};

class Counter extends Component<{}, CounterProps, {}> {
render() : ReactElement<any, any, any> {
let { increment, incrementIfOdd, incrementAsync, decrement, counter } = this.props;
return (
<p>
Clicked: {counter} times
{' '}
<button onClick={increment}>+</button>
{' '}
<button onClick={decrement}>-</button>
{' '}
<button onClick={incrementIfOdd}>Increment if odd</button>
{' '}
<button onClick={() => incrementAsync()}>Increment async</button>
</p>
);
}
}

export default Counter;
33 changes: 33 additions & 0 deletions examples/flow-counter/containers/App.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/* @flow */

import { connect } from 'react-redux';
import Counter from '../components/Counter';
import { increment, decrement, incrementIfOdd, incrementAsync } from '../actions/counter';
import type { AppState, Dispatchable } from '../types';
import type { Dispatch } from 'redux';

type StateProps = AppState;

function mapStateToProps(state : AppState) : StateProps {
return {
counter: state.counter
};
}

type DispatchProps = {
increment: () => any,
decrement: () => any,
incrementIfOdd: () => any,
incrementAsync: () => any
}

function mapDispatchToProps(dispatch: Dispatch<Dispatchable>) : DispatchProps {
return {
increment: () => dispatch(increment()),
decrement: () => dispatch(decrement()),
incrementIfOdd: () => dispatch(incrementIfOdd()),
incrementAsync: () => dispatch(incrementAsync()),
};
}

export default connect(mapStateToProps, mapDispatchToProps)(Counter);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to make this type check correctly, I cannot find a way to specify in the type signature that the resulting props should be the merge of StateProps and DispatchProps from above (both should result in CounterProps that is the props type for the Counter component, but for some reason if I change any of the types from here, it still passes with no errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we ask somebody from Flow team?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think so, I'll create an issue in their repo so that we can get feedback both here and there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just brainstorming a possible solution to work around this problem, but with a big BUT which involves changing the connect API, would be to just have a single function be passed as an argument to connect, which receives the state, dispatch, and the parent props, pretty much like the async functions in redux-thunk.

Just thinking out loud, not necessarily meaning it is the only solution

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. We can't do this because it's performance worst case though. We don't want to bind on every prop or state change unless completely necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're right, forgot about those perf optimizations. Will think a bit more about a possible workaround then, or we can just wait for the Flow team to show us the way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally made this to type check!!! The $Diff type was working, the problem was that the correct flow type for classes that extend React.Component is ReactClass, so that it knows how to infer the polymorphic props and state types

11 changes: 11 additions & 0 deletions examples/flow-counter/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!DOCTYPE html>
<html>
<head>
<title>Redux counter example</title>
</head>
<body>
<div id="root">
</div>
<script src="/static/bundle.js"></script>
</body>
</html>
15 changes: 15 additions & 0 deletions examples/flow-counter/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/* @flow */

import React from 'react';
import { Provider } from 'react-redux';
import App from './containers/App';
import configureStore from './store/configureStore';

const store = configureStore();

React.render(
// Flow limitation: Does not detect children type if we don't explicitly
// set the `children` prop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being tracked here: facebook/flow#892

<Provider store={store} children={() => <App />} />,
document.getElementById('root')
);
16 changes: 16 additions & 0 deletions examples/flow-counter/interfaces/react-redux.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
declare module 'react-redux' {
declare type MapStateToProps<State, Props> = (state: State) => Props;
declare type MapDispatchToProps<Dispatchable, Props> = (dispatch: redux$Dispatch<Dispatchable>) => Props;

declare function connect<State, Dispatchable, StateProps, ComponentProps, DispatchProps: $Diff<ComponentProps, StateProps>, ComponentDefaultProps, ComponentState>(
mapStateToProps: MapStateToProps<State, StateProps>,
mapDispatchToProps: MapDispatchToProps<Dispatchable, DispatchProps>
) : (component: ReactClass<ComponentDefaultProps, ComponentProps, ComponentState>) => ReactClass<ComponentDefaultProps, ComponentProps, ComponentState>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the fix to make connect type check. Using ReactClass instead of Component or ReactComponent. The $Diff internal type works, though it is still internal, so we should pay attention to the evolution of it in Flow


declare type ProviderProps<S, D> = {
store: redux$Store<S, D>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not completely sure why this is happening, and not even sure how to phrase the issue to create in the Flow repo. Seems like a very specific edge case. I'll see if I can find a smaller example to reproduce, and if not file an issue just with this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that import type in the module declarations never worked at all: facebook/flow#905. So for now working with globally declared types

children?: () => ReactElement<any, any, any>
};

declare class Provider<S, D> extends ReactComponent<{}, ProviderProps<S, D>, {}> {}
}
23 changes: 23 additions & 0 deletions examples/flow-counter/interfaces/redux-thunk.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// For now declaring these types globally with a namespace, so I can
// export the default function in the module
declare type reduxThunk$Thunk<State, Dispatchable> = (dispatch: redux$Dispatch<Dispatchable>, getState: () => State) => ?Dispatchable;
declare type reduxThunk$ThunkMiddlewareDispatchable<State, Dispatchable> = reduxThunk$Thunk<State, Dispatchable> | Dispatchable;

declare module 'redux-thunk' {
declare var exports: {
<State, Dispatchable>(
api: redux$MiddlewareAPI<State, reduxThunk$ThunkMiddlewareDispatchable<State, Dispatchable>>
) : (next: redux$Dispatch<Dispatchable>) => redux$Dispatch<reduxThunk$ThunkMiddlewareDispatchable<State, Dispatchable>>
};
}

// function thunk<State, Dispatchable: Object>(_ref: MiddlewareAPI<State, ThunkMiddlewareDispatchable<State, Dispatchable>>) : (next: Dispatch<Dispatchable>) => Dispatch<ThunkMiddlewareDispatchable<State, Dispatchable>> {
// var dispatch = _ref.dispatch;
// var getState = _ref.getState;
//
// return function (next) {
// return function (action: ThunkMiddlewareDispatchable<State, Dispatchable>) : ?ThunkMiddlewareDispatchable<State, Dispatchable> {
// return typeof action === 'function' ? action(dispatch, getState) : next(action);
// };
// };
// }
39 changes: 39 additions & 0 deletions examples/flow-counter/interfaces/redux.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
declare type redux$Dispatch<Dispatchable> = (action: Dispatchable) => ?Dispatchable;

declare type redux$Store<State, Dispatchable> = {
dispatch: redux$Dispatch<Dispatchable>,
subscribe: (listener: () => void) => (() => void),
getState: () => State,
replaceReducer: any
};

declare type redux$MiddlewareAPI<State, Dispatchable> = {
dispatch: redux$Dispatch<Dispatchable>,
getState: () => State,
};

declare type redux$Middleware<State, Dispatchable, DispatchableNext> = (
api: redux$MiddlewareAPI<State, Dispatchable>
) => (
next: redux$Dispatch<DispatchableNext>
) => redux$Dispatch<Dispatchable>;

declare module "redux" {
declare type ReduxInitAction = { type: '@@redux/INIT' };
declare type ReduxAction<Action> = ReduxInitAction | Action;

declare type Reducer<State, Action> = (state?: State, action: ReduxAction<Action>) => State;
declare type Dispatch<Dispatchable> = redux$Dispatch<Dispatchable>;

declare type Store<State, Dispatchable> = redux$Store<State, Dispatchable>;

declare type CreateStore<State, Action, Dispatchable> = (reducer: Reducer<State, Action>, initialState?: State) => Store<State, Dispatchable>
declare function createStore<State, Action>(reducer: Reducer<State, Action>, initialState?: State) : Store<State, ReduxAction<Action>>;

declare type MiddlewareAPI<State, Dispatchable> = redux$MiddlewareAPI<State, Dispatchable>;

declare type Middleware<State, Dispatchable, DispatchableNext> = redux$Middleware<State, Dispatchable, DispatchableNext>;

declare function applyMiddleware<State, Action, Dispatchable, DispatchableNext>(middleware: Middleware<State, Dispatchable, DispatchableNext>)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd need to call applyMiddleware many times to have several middleware while staying typed, right? Seems like a good enough compromise to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can define a couple of overrides, was just testing with one (since the counter example just uses one middleware), but it should be possible to define multiple, as we discussed in the other thread (perhaps up to 5, 6 middleware functions)

: (next: CreateStore<State, Action, DispatchableNext>) => CreateStore<State, Action, Dispatchable>;
}
40 changes: 40 additions & 0 deletions examples/flow-counter/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"name": "redux-flow-counter-example",
"version": "0.0.0",
"description": "Redux Flow counter example",
"scripts": {
"start": "node server.js",
"test": "NODE_ENV=test mocha --recursive --compilers js:babel-core/register",
"test:watch": "npm test -- --watch"
},
"repository": {
"type": "git",
"url": "https://github.com/rackt/redux.git"
},
"license": "MIT",
"bugs": {
"url": "https://github.com/rackt/redux/issues"
},
"homepage": "http://rackt.github.io/redux",
"dependencies": {
"react": "^0.13.3",
"react-redux": "^2.1.2",
"redux": "file:../../",
"redux-thunk": "^0.1.0"
},
"devDependencies": {
"babel-core": "^5.6.18",
"babel-loader": "^5.1.4",
"babel-plugin-react-transform": "^1.0.3",
"expect": "^1.6.0",
"express": "^4.13.3",
"jsdom": "^5.6.1",
"mocha": "^2.2.5",
"mocha-jsdom": "^1.0.0",
"node-libs-browser": "^0.5.2",
"react-transform-hmr": "^1.0.0",
"webpack": "^1.9.11",
"webpack-dev-middleware": "^1.2.0",
"webpack-hot-middleware": "^2.2.0"
}
}
14 changes: 14 additions & 0 deletions examples/flow-counter/reducers/counter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/* @flow */

import type { CounterState, Action } from '../types';

export default function counter(state?: CounterState = 0, action: Action) : CounterState {
switch (action.type) {
case 'INCREMENT_COUNTER':
return state + 1;
case 'DECREMENT_COUNTER':
return state - 1;
default:
return state;
}
}
10 changes: 10 additions & 0 deletions examples/flow-counter/reducers/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/* @flow */

import counter from './counter';
import type { AppState, Action } from '../types';

export default function rootReducer(state?: AppState, action : Action) : AppState {
return {
counter: counter(state && state.counter, action)
};
}
23 changes: 23 additions & 0 deletions examples/flow-counter/server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
var webpack = require('webpack');
var webpackDevMiddleware = require('webpack-dev-middleware');
var webpackHotMiddleware = require('webpack-hot-middleware');
var config = require('./webpack.config');

var app = new require('express')();
var port = 3000;

var compiler = webpack(config);
app.use(webpackDevMiddleware(compiler, { noInfo: true, publicPath: config.output.publicPath }));
app.use(webpackHotMiddleware(compiler));

app.get("/", function(req, res) {
res.sendFile(__dirname + '/index.html');
});

app.listen(port, function(error) {
if (error) {
console.error(error);
} else {
console.info("==> 🌎 Listening on port %s. Open up http://localhost:%s/ in your browser.", port, port);
}
});
27 changes: 27 additions & 0 deletions examples/flow-counter/store/configureStore.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/* @flow */

import { createStore, applyMiddleware } from 'redux';
import thunk from 'redux-thunk';
// import type { ThunkMiddlewareDispatchable } from 'redux-thunk';
import reducer from '../reducers';
import type { AppState, Action } from '../types';

type ThunkMiddlewareDispatchable<S, D> = reduxThunk$ThunkMiddlewareDispatchable<S, D>;

const createStoreWithMiddleware = applyMiddleware(
thunk
)(createStore);

export default function configureStore(initialState?: AppState) {
const store = createStoreWithMiddleware(reducer, initialState);

if (module.hot) {
// Enable Webpack hot module replacement for reducers
module.hot.accept('../reducers', () => {
const nextReducer = require('../reducers');
store.replaceReducer(nextReducer);
});
}

return store;
}
Loading