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.

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

Merged
merged 12 commits into from Feb 26, 2016
Merged

Add TypeScript definitions #1413

merged 12 commits into from Feb 26, 2016

Conversation

aikoven
Copy link
Collaborator

@aikoven aikoven commented Feb 16, 2016

Original issue: #1401

@gaearon
Copy link
Contributor

@gaearon gaearon commented Feb 16, 2016

Do we want somebody else to review these too, or is this good to merge?

@ulfryk
Copy link

@ulfryk ulfryk commented Feb 16, 2016

  1. I'd suggest adding tests for typings (like in DT)
  2. Please take a look at my proposition -> https://gist.github.com/ulfryk/69981ccfb488647b6ee3 .

for example

export function createStore<S>(reducer: Reducer<S>, initialState?: S, enhancer?: StoreEnhancer): Store<S>;

does not give strict rules of use, look at this instead:

interface IStoreCreator extends Function {
    <S>(reducer: IReducer<S>): Store<S>;
    <S>(reducer: IReducer<S>, enhancer: IStoreEnhancer<S>): Store<S>;
    <S>(reducer: IReducer<S>, initialState: S): Store<S>;
    <S>(reducer: IReducer<S>, initialState: S, enhancer: IStoreEnhancer<S>): Store<S>;
}
…
interface IStoreEnhancer<S> extends Function {
    (createStore: IStoreCreator): IStoreCreator;
}
…
const createStore: IStoreCreator;

etc. etc.

Of course I'm not talking about naming convention (ignore I prefix if you want). What is the most useful in my proposition - it's the strict set of rules how createStore() can be used.

@gaearon
Copy link
Contributor

@gaearon gaearon commented Feb 16, 2016

If we can add tests for these, we should add tests.


export type Reducer<S> = (state: S, action: any) => S;

export type Dispatch = (action: any) => any;
Copy link

@ulfryk ulfryk Feb 16, 2016

Choose a reason for hiding this comment

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

It would be great if try to not use too much any - it's just like working without support of typscript… :(

Consider more strict approach like:

interface Action {
    type: any;
}

interface Reducer<S> extends Function {
    <S>(state: S, action: Action): S;
}

interface ReducersMapObject<S> {
    [key: string]: Reducer<S>
}

interface Dispatch extends Function {
    <A extends Action>(action: A): A;
}

Or even better

interface Action<T> {
    type: T;
}

interface Reducer<S, T> extends Function {
    <S>(state: S, action: Action<T>): S;
}
…
interface Dispatch<T> extends Function {
    <A extends Action<T>>(action: A): A;
}

so anyone can implement his own strict actions:

enum ActionType {A, B, C}
interface MyAction implements Redux.Action<ActionType> {
    type: ActionType;
    payload: MyPayloadType;

or dynamic ones:

interface MyAction implements Redux.Action<any> {}

Copy link
Collaborator Author

@aikoven aikoven Feb 17, 2016

Choose a reason for hiding this comment

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

Why isn't this sufficient?

interface Action {
  type: string;
}

If you want to have strict actions you can just do:

enum ActionType {A, B, C}
interface MyAction<P> {
    type: ActionType;
    payload: P;
}

And it will be assignable to Action.

Also, reducer should not be constrained to accept only some selected actions. It should accept any possible action and bypass ones it doesn't need to handle.

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Feb 16, 2016

@ulfryk As far as I can see from createStore source, enhanced store creator has only one signature:
https://github.com/reactjs/redux/blob/master/src%2FcreateStore.js#L49

return enhancer(createStore)(reducer, initialState)

But you are right about original createStore signatures. I'll make a fix.

You are also right about type constraint on action argument for Reducer.
But Dispatch can accept argument of any type, not only Action, if user uses some middleware.

@gaearon DefinitelyTyped uses TypeScript code to see if it compiles correctly against definitions. I've added tests for original PR: https://github.com/aikoven/DefinitelyTyped/blob/redux/redux/redux-tests.ts. But I'll need to figure out how to actually run them here.

Will do it tomorrow.

@ulfryk
Copy link

@ulfryk ulfryk commented Feb 16, 2016

@aikoven

Enhanced creator

You're right - enhancer has only one signature. But its signature (of course without <S>) is reusable:

interface IStoreEnhancer extends Function {
    (createStore: IStoreCreator): IStoreCreator;
}

here:

function applyMiddleware<S>(...middlewares: IMiddleware<S>[]): IStoreEnhancer;

and here:

interface IStoreCreator extends Function {
    <S, A>(reducer: Reducer<S, A>): Store<S>;
    <S, A>(reducer: Reducer<S, A>, enhancer: IStoreEnhancer): Store<S>;
    <S, A>(reducer: Reducer<S, A>, initialState: S): Store<S>;
    <S, A>(reducer: Reducer<S, A>, initialState: S, enhancer: IStoreEnhancer): Store<S>;
}

Reducer

It can be still a generic type:

interface Reducer<S, A> extends Function {
    <S, A>(state: S, action: A): S;
}

interface IStoreCreator extends Function {
    <S, A>(reducer: Reducer<S, A>): Store<S>;
   …

so anyone who uses some middleware and need many types of action-like-things can use

const rootReducer = (state: MyState, a: any): MyState => {…};
const myStore = createStore(rootReducer);

@mikekidder
Copy link
Contributor

@mikekidder mikekidder commented Feb 17, 2016

Has this PR been compared to what has already been submitted to typings/registry?

Which one takes precedence if someone does "typings search redux" not realizing a definition file exists in Redux repo? I can see it bringing confusion, and in turn, issues opened on Redux repo.

@@ -0,0 +1,60 @@
export interface Action {
type: string;
Copy link
Collaborator Author

@aikoven aikoven Feb 17, 2016

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 be any?

Copy link

@ulfryk ulfryk Feb 17, 2016

Choose a reason for hiding this comment

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

Maybe

export interface Action<T> {
    type: T;
}

Copy link
Collaborator Author

@aikoven aikoven Feb 17, 2016

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:

enum ActionType {
   A, B, C
}

But where would it be useful? We can use it in reducer:

function reducer(state, action: Action<ActionType>) {
  // ...
}

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.

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Feb 17, 2016

@mikekidder I guess once this PR gets merged it will become official typings for Redux and Typings registry should point to it.

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Feb 17, 2016

Added some tests using https://github.com/adamcarr/typescript-definition-tester.
For now it requires to add typescript and chai to devDependencies, until this is resolved: adamcarr/typescript-definition-tester#1


export interface Middleware {
<S>(api: MiddlewareAPI<S>): (next: Dispatch) => Dispatch;
}
Copy link

@ulfryk ulfryk Feb 17, 2016

Choose a reason for hiding this comment

The 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 Middleware will always have to be parametrised with both type parameters. We can avoid this ( but I'm NOT sure if we should ) in the same manner as with Dispatch:

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… ;)

Copy link
Collaborator Author

@aikoven aikoven Feb 17, 2016

Choose a reason for hiding this comment

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

Let's try to implement thunk middleware in TypeScript as an example. Start without any types:

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 thunk, so dispatch here can potentially accept anything, e.g. promises. Same for next, same for action.
Middleware is standalone and doesn't know anything about dispatch type prior to when it was applied.

Copy link

@ulfryk ulfryk Feb 17, 2016

Choose a reason for hiding this comment

The 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 ?

Copy link

@ulfryk ulfryk Feb 17, 2016

Choose a reason for hiding this comment

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

It shows that elasticity of thunkMiddleware stays in oposition to type safety. Anyway it can be always done in that way:

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)
    }

;)

Copy link
Collaborator Author

@aikoven aikoven Feb 18, 2016

Choose a reason for hiding this comment

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

The problem is — thunkMiddleware is standalone and it can't have knowledge of what MyStore is.

@gaearon
Copy link
Contributor

@gaearon gaearon commented Feb 17, 2016

@aikoven @ulfryk

I added you both to collaborators with write access and leave this one to you 😉 . Please work it out between yourselves how/whether you’d like to collaborate, and merge this when you think it’s good enough. I also trust you to maintain these and will ping you if we introduce any changes to the API.

Thanks!

@ulfryk
Copy link

@ulfryk ulfryk commented Feb 17, 2016

@gaearon - Thank you very much for trust :) We'll try to do our best!

More strict types: replace `any` with type parameters
Add middleware tests
@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Feb 18, 2016

@ulfryk Updated typings. Please also see comments for outdated diffs.

I have one concern for Action type yet. It's clear that user will have to extend Action to describe shape of actions in their app. So if we have

interface Action {
  type: string;
}

then user can have e.g. FSA like this:

interface MyAction<P> extends Action {
  payload: P;
}

or free-shape action:

interface MyAction extends Action {
  [key: string]: any;
}

If you want to constraint type property:

type ActionType = 'A' | 'B' | 'C';

interface MyAction extends Action {
  type: ActionType;
  // ...
}

Although if ActionType was enum, this wouldn't compile, because String Enums aren't supported yet. We could loosen constraint for type to be any, but I'm not sure if we should: values of enum are integers, which would make it hard to analyze action log with DevTools.

Still Redux docs don't enforce type to be string:

Types should typically be defined as string constants.

cc @gaearon

@ulfryk
Copy link

@ulfryk ulfryk commented Feb 18, 2016

@aikoven - about Action interface - we have 2 possibilities:

Strict

interface Action<T> {
    type: T;
}
// So user will always have to specify type of `type` property

then user can have e.g. FSA like this:

interface MyAction<P> extends Action<string> {
  payload: P;
}

// or

interface MyAction<P, T> extends Action<T> {
  payload: P;
}

or free-shape action:

interface MyAction extends Action<string> {
  [key: string]: any;
}

If you want to constraint type property:

type ActionType = 'A' | 'B' | 'C';

interface MyAction extends Action<ActionType> {
  // ...
}

and TS enums will also work:

type ActionType = { A, B, C }

interface MyAction extends Action<ActionType> {
  // ...
}

or

Elastic

interface Action {
    type: any;
}

It works in all your examples plus TS enums, but User always have to define his own interface/type for actions if he want's stricter mode.

Conclusion

IMO stricter version is much better:

interface Action<T> {
    type: T;
}


export type Reducer<S> = <A extends Action>(state: S, action: A) => S;

export function combineReducers<S>(reducers: {[key: string]: Reducer<any>}): Reducer<S>;
Copy link

@ulfryk ulfryk Feb 18, 2016

Choose a reason for hiding this comment

The 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 State type if one was defined:

interface MyReducers extends ReducersMapObject {
    'posts': Reducer<Post[]>;
    'user': Reducer<MyUser>;
     
}

const reducers: MyReducers = ;

const myRootReducer = combineReducers<Reducer<State>, MyReducers>(reducers);

Copy link
Collaborator Author

@aikoven aikoven Feb 18, 2016

Choose a reason for hiding this comment

The 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>;

Copy link

@ulfryk ulfryk Feb 18, 2016

Choose a reason for hiding this comment

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

Overloading makes sense 👍

But have to be well tested ;)

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Mar 21, 2016

Please note #1526

@Igorbek
Copy link
Contributor

@Igorbek Igorbek commented Mar 22, 2016

This too #1537

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Mar 31, 2016

For everyone concerned with stronger typings for dispatch, please see @Igorbek's proposal: #1537.

It's based on module/interface augmentation introduced in TS 1.8. So to add dispatch signature for e.g. redux-thunk, you would do:

// redux-thunk.d.ts

declare module "redux" {
    export interface Dispatch<S> {
        <R>(asyncAction: (dispatch: Dispatch<S>, getState: () => S) => R): R;
    }
}

This looks like a decent solution to me, but I'd like to hear from others before merging.

@gaearon
Copy link
Contributor

@gaearon gaearon commented Apr 2, 2016

@aikoven

Sorry I haven’t been maintaining Redux closely for the past couple of months.
What’s up with these? Are we good to cut a minor release with this, or shall we wait for more changes?

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Apr 4, 2016

@gaearon IMO we should wait at least for #1537.
Also there's a discussion #1526 considering StoreEnhancer/StoreCreator types.

@gaearon gaearon added this to the 4.0 milestone Apr 4, 2016
@Ciantic
Copy link

@Ciantic Ciantic commented Apr 4, 2016

Would it be impossible to throw the contents of these: https://github.com/reactjs/redux/blob/master/index.d.ts to inside the declare namespace in here: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/redux/redux.d.ts

I had to do it manually couple of times already.

Currently most people are getting their types from there, at least it would match the official types in upcoming release this way.

@gaearon
Copy link
Contributor

@gaearon gaearon commented Apr 4, 2016

@aikoven OK. Ping me here or, if I miss it, on Twitter, when you’re ready to cut it.

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Apr 4, 2016

@Ciantic We'll probably have to copy these typings to DT, because it seems there's no other way to treat other typings in DT that are dependent on Redux.

@Igorbek
Copy link
Contributor

@Igorbek Igorbek commented Apr 4, 2016

It makes sense, just to allow seamless transition between these twos. I'll create a PR there and once you make latest redux official I'll merge it.

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Apr 5, 2016

@gaearon We're good to go

@gaearon
Copy link
Contributor

@gaearon gaearon commented Apr 5, 2016

@aikoven What’s the latest consensus, is doing this in a minor bump okay?

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Apr 5, 2016

I've tested for my build: DT typings take precedence over these, so they don't break anything. New Redux typings seem to be ignored unless user takes some steps i.e. manually unlink other dependent libraries' typings from old DT Redux typings so that TS compiler could resolve imports from redux to new ones.

So yes, it is safe to release this as a minor.

@gaearon
Copy link
Contributor

@gaearon gaearon commented Apr 5, 2016

What if the user had no typings at all? Is this possible in TypeScript?

@Igorbek
Copy link
Contributor

@Igorbek Igorbek commented Apr 5, 2016

@gaearon if user hadn't DT typings, the typing shipped with the module would be resolved. That said, a user that just installed 'redux' module can start using TypeScript without need to install DT or other external typings. See Typings in NPM modules in TypeScript Wiki

@gaearon
Copy link
Contributor

@gaearon gaearon commented Apr 5, 2016

Then this is a breaking change.

  1. User has no typings
  2. We add typings in a minor version
  3. Their build process fails

Is this correct?

@Igorbek
Copy link
Contributor

@Igorbek Igorbek commented Apr 5, 2016

I don't think it will fail. If user had no typings it wouldn't be able to use any explicit redux types.

@Igorbek
Copy link
Contributor

@Igorbek Igorbek commented Apr 5, 2016

Anyway, I might misunderstand and if it's possible to break a build somehow, it's a breaking change that needs major.

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Apr 6, 2016

Can we then bump major without waiting until other issues from 4.0 milestone are resolved? There's increasing number of people who want this released asap. How about RC version?

@Igorbek
Copy link
Contributor

@Igorbek Igorbek commented Apr 6, 2016

Personally, I'm pretty sure that adding typing bindings is a non-breaking change that theoretically might break build in very synthetic case. Anyway, I'm not insisting.

@Ciantic
Copy link

@Ciantic Ciantic commented Apr 6, 2016

  1. User has no typings
  2. We add typings in a minor version
  3. Their build process fails

Then the user is not using TypeScript, because it's not possible to go past point number one on this list.

Because you can't use TypeScript without typings, they get big honking error message if they try to use redux without typings. It just doesn't compile until the error message is handled, and they have to either write any declaration themselves or install them from DT.

Though, most of the people using TS has to deal with wildly changing typings anyway, so if there is some hypothethical case where someone has used redux without typings, and if typings entry appears it suddenly uses TS, it's a weird one.

@ulfryk
Copy link

@ulfryk ulfryk commented Apr 6, 2016

I 100% agree with @Ciantic

@aikoven
Copy link
Collaborator Author

@aikoven aikoven commented Apr 8, 2016

@gaearon I guess the consensus so far is that bumping major is not necessary here.

@gaearon gaearon removed this from the 4.0 milestone Apr 8, 2016
@gaearon
Copy link
Contributor

@gaearon gaearon commented Apr 8, 2016

Out in 3.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet