Skip to content
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.

Sign up for GitHub

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

Jump to bottom

TypeScript definitions improvements #1526

Merged
merged 1 commit into from
Apr 4, 2016
Merged

TypeScript definitions improvements #1526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 8 additions & 2 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ export function combineReducers<S>(reducers: ReducersMapObject): Reducer<S>;

/* store */

export interface MiddlewareDispatch {
<TMiddlewareAction, TMiddlewareActionResult>(action: TMiddlewareAction): TMiddlewareActionResult;
Copy link
Contributor

Choose a reason for hiding this comment

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

it will not infer types automatically. can you define the function that will not require providing types explicitly?
If I call it without types, TS will infer {} as a TMiddlewareAction.

}

/**
* A *dispatching function* (or simply *dispatch function*) is a function that
* accepts an action or an async action; it then may or may not dispatch one
Expand All @@ -93,7 +97,9 @@ export function combineReducers<S>(reducers: ReducersMapObject): Reducer<S>;
* transform, delay, ignore, or otherwise interpret actions or async actions
* before passing them to the next middleware.
*/
export type Dispatch = (action: any) => any;
export interface Dispatch extends MiddlewareDispatch {
(action: Action): Action;
}

/**
* Function to remove listener added by `Store.subscribe()`.
Expand Down Expand Up @@ -265,7 +271,7 @@ export interface MiddlewareAPI<S> {
* asynchronous API call into a series of synchronous actions.
*/
export interface Middleware {
<S>(api: MiddlewareAPI<S>): (next: Dispatch) => (action: any) => any;
<S>(api: MiddlewareAPI<S>): (next: MiddlewareDispatch) => MiddlewareDispatch;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion test/typescript/dispatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ const dispatchResult: Action = dispatch({type: 'TYPE'});

type Thunk<O> = () => O;

const dispatchThunkResult: number = dispatch(() => 42);
const dispatchThunkResult: number = dispatch<Thunk<number>, number>(() => 42);
Copy link
Contributor

Choose a reason for hiding this comment

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

that's what I'm talking about.

Copy link

Choose a reason for hiding this comment

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

Could be written

const dispatchThunkResult: number = dispatch((() => 42) as Thunk<number>);

which is shorter, more explicit and clear of what it does, provides exactly the same type safety and doesn't require people to put type casts all over their code if they doesn't want to.

Also, as you know what middlewares you have configured, for extra type safety you can cast the dispatch function instead (safer because you would only need to do this once so you drastically reduce the risk of miss typing).

declare const dispatch: <T>(thunk: Thunk<T>) => T;

As long as their isn't a way to make the types flow through all middlewares, that is the safest way. Adding type parameters which needs to be specified does not increase the safety at all as there is nothing that guarantees them to be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Pajn , the point of generics is to provide typings over something that is user-defined, in this case the action and the result. While being able to infer them would be ideal, it is still just a convenience.

Could be written

The argument is purely cosmetic and the increase of readability is arguable. It may sound a bit harsh, but which example being shorter is irrelevant, because that's not our goal. Otherwise we would be writing pure JavaScript instead. The main point here is that we end up with something sane, instead of an implicit any that can just leak all over the application. I shot myself in the foot more than a few times with loose definitions and I can tell you that it's better to have:

getSomething<T>(): T;

than

getSomething(): any;

While neither guarantees that you actually cast to the correct type, the first one at least ensures that you made a conscious decision, instead of accepting any by default.