-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Add TypeScript definitions #1413
Changes from 5 commits
889030c
c01501f
4b09a3b
6b4f7f6
b19ebc1
a6b4d80
56ab522
1fa6636
6b61fca
16325a2
6317727
a5d44fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
export interface Action { | ||
type: string; | ||
} | ||
|
||
|
||
/* reducers */ | ||
|
||
export type Reducer<S> = <A extends Action>(state: S, action: A) => S; | ||
|
||
export function combineReducers<S>(reducers: {[key: string]: Reducer<any>}): Reducer<S>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that reducers map should be defined separately: export interface ReducersMapObject {
[key: string]: Reducer<any>;
}
export function combineReducers<S, M extends ReducersMapObject>(reducers: M): Reducer<S>; So anyone can easily make his own type for this corresponding with his interface MyReducers extends ReducersMapObject {
'posts': Reducer<Post[]>;
'user': Reducer<MyUser>;
…
}
const reducers: MyReducers = …;
const myRootReducer = combineReducers<Reducer<State>, MyReducers>(reducers); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see one drawback here: until TypeScript supports default type parameters we'd have to always specify second parameter which adds verbosity. How about overloads: export function combineReducers<S>(reducers: ReducersMapObject): Reducer<S>;
export function combineReducers<S, M extends ReducersMapObject>(reducers: M): Reducer<S>; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overloading makes sense 👍 But have to be well tested ;) |
||
|
||
|
||
/* store */ | ||
|
||
export interface Dispatch { | ||
<A>(action: A): A; | ||
<A, B>(action: A): B; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Middleware definition below seems to be enough, but… interface Middleware<S> extends Function {
(store: MiddlewareAPI<S>): (next: Dispatch) => Dispatch;
} It actually does not return a dispatch but function mapping input of type A to output of type B: interface Middleware<S, A, B> extends Function {
(store: MiddlewareAPI<S>): (next: Dispatch) => (action: A) => B;
} But in this case interface Middleware extends Function {
<S, A, B>(store: MiddlewareAPI<S>): (next: Dispatch) => (action: A) => B;
} It's not always so easy to add static type definitions to code written in dynamically typed language… ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's try to implement const thunkMiddleware = ({dispatch, getState}) =>
(next) => (action) => {
return typeof action === function ? action(dispatch, getState) : next(action)
} Now what types can we add here? Keep in mind that there may me other middlewares applied before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. // in our typings
interface Middleware<S, A, B> extends Function {
(store: MiddlewareAPI<S>): (next: Dispatch) => (action: A) => B;
} import { MyState } from './wherever-my-state-is-declared'
type ThunkAction = ((d: Dispatch, gs: () => MyState) => ThunkAction) | Object;
const thunkMiddleware: Middleware<MyStore, ThunkAction, ThunkAction> =
({dispatch, getState}) =>
(next) => (action) => {
return typeof action === function ? action(dispatch, getState) : next(action)
} Does it do the job ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shows that elasticity of import { MyState } from './wherever-my-state-is-declared';
const thunkMiddleware: Middleware<MyStore, any, any> =
({dispatch, getState}) =>
(next) => (action) => {
return typeof action === function ? action(dispatch, getState) : next(action)
} ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is — |
||
|
||
export interface Store<S> { | ||
dispatch: Dispatch; | ||
getState(): S; | ||
subscribe(listener: () => void): () => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One more lil suggestion here - lets create type/interface for function returned from subscribe so user can declare variable which will hold reference to it in future: // index.d.ts
export interface Unsubscribe extends Function {
(): void;
}
export interface Store<S> {
…
subscribe(listener: () => void): Unsubscribe;
…
}
// real example from my code:
@Component({selector: 'my-app', templateUrl: '/src/app.html'})
export class App implements OnInit, OnDestroy {
constructor(@Inject('AppStore') private appStore: Store<IAppState>) {}
…
// I can declare it with proper type;
private unsubscribe: Unsubscribe;
// and assign value later
public ngOnInit(): void {
this.unsubscribe = this.appStore.subscribe(() => …doSomethingWith(this.appStore.getState())… );
}
public ngOnDestroy(): void {
this.unsubscribe();
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes sense. One question: what's the reasoning behind There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not test it but i think that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've checked, call signature is enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
replaceReducer(reducer: Reducer<S>): void; | ||
} | ||
|
||
export interface StoreCreator { | ||
<S>(reducer: Reducer<S>): Store<S>; | ||
<S>(reducer: Reducer<S>, initialState: S): Store<S>; | ||
|
||
<S>(reducer: Reducer<S>, enhancer: StoreEnhancer): Store<S>; | ||
<S>(reducer: Reducer<S>, initialState: S, enhancer: StoreEnhancer): Store<S>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These can be simplified to export interface StoreCreator {
<S>(reducer: Reducer<S>, enhancer?: StoreEnhancer): Store<S>;
<S>(reducer: Reducer<S>, initialState: S, enhancer?: StoreEnhancer): Store<S>;
} while still providing the same type information and it better describes the actual API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
} | ||
|
||
export type StoreEnhancer = (next: StoreCreator) => StoreCreator; | ||
|
||
export const createStore: StoreCreator; | ||
|
||
|
||
/* middleware */ | ||
|
||
export interface MiddlewareAPI<S> { | ||
dispatch: Dispatch; | ||
getState(): S; | ||
} | ||
|
||
export interface Middleware { | ||
<S>(api: MiddlewareAPI<S>): (next: Dispatch) => <A, B>(action: A) => B; | ||
} | ||
|
||
export function applyMiddleware(...middlewares: Middleware[]): StoreEnhancer; | ||
|
||
|
||
/* action creators */ | ||
|
||
export interface ActionCreator { | ||
<O>(...args: any[]): O; | ||
} | ||
|
||
export function bindActionCreators< | ||
T extends ActionCreator|{[key: string]: ActionCreator} | ||
>(actionCreators: T, dispatch: Dispatch): T; | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Action creators:I think that overloading is much better than union: // or maybe "<A extends Action>" but due to middleware issues better without "extends Action"
export interface ActionCreator<A> extends Function {
(...args: any[]): A;
}
export interface ActionCreatorsMapObject {
[key: string]: <A>(...args: any[]) => A;
}
export interface ActionCreatorsBinder {
<A>(actionCreator: ActionCreator<A>, dispatch: Dispatch): ActionCreator<A>;
<M extends ActionCreatorsMapObject>(actionCreators: M, dispatch: Dispatch): M;
(actionCreators: ActionCreatorsMapObject, dispatch: Dispatch): ActionCreatorsMapObject;
}
export function bindActionCreators: ActionCreatorsBinder; I've tested it with such piece of useless code: const oneCreatorA = bindActionCreators<Action>(a => a, store.dispatch);
const oneCreatorB = bindActionCreators(oneCreatorA, store.dispatch);
const oneCreatorC = bindActionCreators(oneCreatorB, store.dispatch);
interface Ott extends ActionCreatorsMapObject {
one: IActionCreator<Action>;
two: IActionCreator<Action>;
three: IActionCreator<Action>;
four: IActionCreator<Action>;
}
const xCreatorA = bindActionCreators({
one: a => a,
two(b: Action) {return b;},
three: oneCreatorA
}, store.dispatch);
const cMap: Ott = {
one: a => a,
two(b: Action) {return b;},
three: oneCreatorA,
four: oneCreatorC
};
const xCreatorB: ActionCreatorsMapObject = bindActionCreators(xCreatorA, store.dispatch);
const xCreatorC: Ott = bindActionCreators(cMap, store.dispatch); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Third overload seems redundant here, we can omit type parameter in second overload and it will be inferred as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also consider the difference between <A>(actionCreator: ActionCreator<A>, dispatch: Dispatch): ActionCreator<A>; and <A extends ActionCreator<any>>(actionCreator: A, dispatch: Dispatch): A; I guess the latter is stronger because let's you constraint not only return type of ActionCreator, but its full signature including argument types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Second one here: <A extends ActionCreator<any>>(actionCreator: A, dispatch: Dispatch): A; is not typesafe as it enforces I think we should find better way… There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aikoven also you said
And that is great :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. This signature allows doing bindActionCreators<ActionCreator<MyAction>>(...) and even bindActionCreators<(text: string) => MyAction>(...) So type parameter is not enforced. In contrast, if we used this signature: <A>(actionCreator: ActionCreator<A>, dispatch: Dispatch): ActionCreator<A>; to bind action creator of type Also, I found that we don't cover cases when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you're right. My mistake: <A extends ActionCreator<any>>(actionCreator: A, dispatch: Dispatch): A; Is totally ok.
So maybe third overload: <A extends ActionCreator<any>, B extends ActionCreator<any>>(
actionCreator: A,
dispatch: Dispatch
): B; |
||
/* compose */ | ||
|
||
// from DefinitelyTyped/compose-function | ||
// Hardcoded signatures for 2-4 parameters | ||
export function compose<A, B, C>(f1: (b: B) => C, | ||
f2: (a: A) => B): (a: A) => C; | ||
export function compose<A, B, C, D>(f1: (b: C) => D, | ||
f2: (a: B) => C, | ||
f3: (a: A) => B): (a: A) => D; | ||
export function compose<A, B, C, D, E>(f1: (b: D) => E, | ||
f2: (a: C) => D, | ||
f3: (a: B) => C, | ||
f4: (a: A) => B): (a: A) => E; | ||
|
||
// Minimal typing for more than 4 parameters | ||
export function compose<I, R>(f1: (a: any) => R, | ||
...functions: Function[]): (a: I) => R; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The most inner and the returned function does not necessarily take just a single argument, see https://github.com/reactjs/redux/pull/1399/files So for all these the returned function must have an untyped parameterlist of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like this maybe:
and
It is really hard to type it strictly… Maybe it should be done just like tuples are in Scala - with limit to ~24 composed functions ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But then you assume that the rightmost function takes at least one argument, which might be false. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import * as tt from 'typescript-definition-tester' | ||
|
||
|
||
describe('TypeScript definitions', () => { | ||
it('should compile against index.d.ts', (done) => { | ||
tt.compileDirectory( | ||
__dirname + '/typescript', | ||
fileName => fileName.match(/\.ts$/), | ||
() => done() | ||
) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import {createStore, Action} from '../../index.d.ts' | ||
|
||
interface AddTodoAction extends Action { | ||
text: string; | ||
} | ||
|
||
function todos(state: string[] = [], action: Action): string[] { | ||
switch (action.type) { | ||
case 'ADD_TODO': | ||
const addTodoAction = <AddTodoAction>action; | ||
return state.concat([addTodoAction.text]); | ||
default: | ||
return state | ||
} | ||
} | ||
|
||
let store = createStore(todos, ['Use Redux']); | ||
|
||
store.dispatch({ | ||
type: 'ADD_TODO', | ||
text: 'Read the docs' | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import { | ||
applyMiddleware, createStore, | ||
Middleware, Dispatch | ||
} from "../../index.d.ts"; | ||
|
||
export const loggerMiddleware: Middleware = ({getState}) => { | ||
return (next: Dispatch) => | ||
(action: any) => { | ||
console.log('will dispatch', action) | ||
|
||
// Call the next dispatch method in the middleware chain. | ||
let returnValue = next(action) | ||
|
||
console.log('state after dispatch', getState()) | ||
|
||
// This will likely be the action itself, unless | ||
// a middleware further in chain changed it. | ||
return returnValue | ||
} | ||
} | ||
|
||
function todos(state: any, action: any) { | ||
return state; | ||
} | ||
|
||
let store = createStore( | ||
todos, | ||
['Use Redux'], | ||
applyMiddleware(loggerMiddleware) | ||
); | ||
|
||
store.dispatch({ | ||
type: 'ADD_TODO', | ||
text: 'Understand the middleware' | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import {createStore} from "../../index.d.ts"; | ||
|
||
const store = createStore((state: any) => state, { | ||
some: {deep: {property: 42}} | ||
}); | ||
|
||
function select(state: any) { | ||
return state.some.deep.property | ||
} | ||
|
||
let currentValue: number; | ||
function handleChange() { | ||
let previousValue = currentValue; | ||
currentValue = select(store.getState()); | ||
|
||
if (previousValue !== currentValue) { | ||
console.log('Some deep nested property changed from', previousValue, 'to', currentValue) | ||
} | ||
} | ||
|
||
let unsubscribe = store.subscribe(handleChange); | ||
handleChange(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import { | ||
Middleware, MiddlewareAPI, | ||
applyMiddleware, createStore, Dispatch | ||
} from "../../index"; | ||
|
||
|
||
type Thunk<S> = <O>(dispatch: Dispatch, getState?: () => S) => O; | ||
|
||
|
||
const thunkMiddleware: Middleware = | ||
<S>({dispatch, getState}: MiddlewareAPI<S>) => | ||
(next: Dispatch) => | ||
<A>(action: A | Thunk<S>) => | ||
typeof action === 'function' ? | ||
(<Thunk<S>>action)(dispatch, getState) : | ||
next(action) | ||
|
||
|
||
function todos(state: any, action: any) { | ||
return state; | ||
} | ||
|
||
let store = createStore( | ||
todos, | ||
['Use Redux'], | ||
applyMiddleware(thunkMiddleware) | ||
); | ||
|
||
store.dispatch<Thunk<any>, void>((dispatch: Dispatch) => { | ||
dispatch({type: 'ADD_TODO'}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? Do we expect action type to always be
string
, or should it beany
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only case I see for this is your example with
enum
:But where would it be useful? We can use it in reducer:
But that would be incorrect:
action
argument here is not only your user-defined actions, it can also be{type: '@@redux/INIT'}
or any other action used by third-party redux library.