TypeScript rewrite

[denisw]

Based on the Babel 7 PR (#71).

The codebase will be ported here incrementally based on #38, #70 and @Dudeonyx's TypeScript fork.

To do:

• createAction.js
• createReducer.js
• configureStore.js
• isPlainObject.js
• createSlice.js
• index.js
• serializableStateInvariantMiddleware.js
• sliceSelector.js
• Add type tests using typings-tester (also used in the core Redux repo)

[netlify]

Deploy preview for redux-starter-kit-docs ready!

Built with commit 497babf

https://deploy-preview-73--redux-starter-kit-docs.netlify.com

[denisw]

@Dudeonyx I just finished turning everything to TypeScript. I would really appreciate a review by you, especially regarding the interfaces and type arguments used (they are a bit different from the ones you used in your fork).

I also added some typings-tester tests that make it easy to verify that e.g. type argument inference works the way we envision.

[denisw]

@markerikson In this PR I added doc comments to all functions so that they show up during auto-completion in Visual Studio Code and other editors. These comments are taken from the current docs, but are modified and extended a bit. I would be happy to backport these improvements to the docs pages (likely in a separate PR) if they look good to you.

[resir014]

First pass of reviews. Looks good so far, might come back here if I see more.

[Jessidhia]

I wrote these mostly from the point of view of a consuming the API; I'm not sure if some of the things I commented on are non-public APIs.

[Jessidhia]

This erases the type of the thunk middleware, so you need to somehow add it back later, otherwise its enhanced dispatch type gets lost.

[Dudeonyx]

Doing it like this

const IS_PRODUCTION = process.env.NODE_ENV === 'production';

export function getDefaultMiddleware(isProduction = IS_PRODUCTION) {
  const middlewareArray = [thunk,];
  let middlewareArrayPlus;

  if (!isProduction) {
    middlewareArrayPlus = [
      createImmutableStateInvariantMiddleware(),
      thunk,
      createSerializableStateInvariantMiddleware(),
    ];
  }

  return middlewareArrayPlus === undefined
    ? middlewareArray
    : middlewareArrayPlus;
}

preserves the types

[denisw]

Hm, yeah, this could be [ThunkMiddleware<S, A>, ...Middleware[]] for instance. However I have no idea how we would preserve the extension type in configureStore. Would we need overrides with a one-element middleware tuple, two-element middleware tuple, etc., so that we can write Store<S,A> & Ext1 & Ext2 & ...?

[Dudeonyx]

yeah, we would need to write an exponential amount of overloads since not only is the number of middlewares variable the number of enhancers is as well.
So we'd end up with overrides for one-element middleware tuple and one-element enhancer tuple, one-element middleware tuple and two-element enhancer tuple, two-element middleware tuple and one-element enhancer tuple, two-element middleware tuple and two-element enhancer tuple, etc

[denisw]

Yes, I think this is a point where we need to favor practicality over maximum type precision. If we cannot find an economical way to get the type just right, we'll probably get by just fine by having the configured store's dispatch accept a relatively generic type like unknown for now. We can later see if we can tweak the API to make typing easier.

[denisw]

For now, I addressed the problem by letting configureStore() return an EnhancedStore extends Store type that is hardcoded to allow dispatching thunks in addition to plain actions. This at least unblocks the use of redux-thunk. Let's improve on this after this PR (perhaps with the help of an API change).

[denisw]

@Kovensky Thank you a lot for the very thorough review! I'm still learning capture and maintain as much type information as practical, so your input is extremely appreciated.

I'll comment on your points one-by-one. On a general note, this PR is deliberately about a one-to-one translation to TypeScript without any changes to the API design as such (which surely could be made more typing-friendly at some places). Improving the API for type-ability is then the next step after this has been merged.

[markerikson]

I've just published https://github.com/reduxjs/redux-starter-kit/releases/tag/v0.4.0-0 as NPM tag ts-next. Let's get a few people to try it out and see if there are any issues!

[phryneas]

neat, but in the current release on ts-next, the index.d.ts references src/... files, which are not distributed (only dist included)

could you please fix that?

[markerikson]

Cool. I'll put up a new @ts-next release this evening.

[markerikson]

@denisw : I don't think the index.d.ts changes are correct. Right now the typings tests are failing because it can't import things into index.d.ts. We do single-file builds, so I'm pretty sure there's no dist/createActions.js, etc.

[denisw]

@markerikson The prepare script (specifically, tsc:declarations) generates a.d.ts file for every .ts file (excluding tests). So while there will be no dist/createActions.js, there should be dist/createActions.d.ts for importing from index.d.ts (such imports "just work").

[denisw]

Just noticed that my declaration file approach broke the typing tests. I'll reverted the change and resorted to simply adding src/ to "files" in package.json.

[markerikson]

Published v0.4.0-1 as tag ts-next. How's it look?

[markerikson]

Awright, no one's saying "it works", but no one's whining about this either. Let's do it.

[markerikson]

Published as https://github.com/reduxjs/redux-starter-kit/releases/tag/v0.4.0 . Thanks to everyone for working on this!

[christoph-jerolimov]

It works! Thanks @denisw and @markerikson

[vincentjames501]

Thanks @denisw and @markerikson , this is great!

From the looks of the original PR message, is the createSlice type definitions completely finalized? I've been using it the last few days and bumping into a few issues with TypeScript (comments inline).

import { configureStore, createSlice } from 'redux-starter-kit';
import { combineReducers } from 'redux';
// Is there a better way to import/use this type? Pulling from `/src` doesn't feel right
import { PayloadAction } from 'redux-starter-kit/src/createAction';

const user = createSlice({
  slice: 'user',
  initialState: { name: '', age: 100 },
  reducers: {
    setUserName: (state, action: PayloadAction<string>) => {
      state.name = action.payload;
    },
  },
});

const store = configureStore({
  reducer: combineReducers({
    user: user.reducer,
  }),
});
// This results in a compilation issue
// const store2 = configureStore({
//   reducer: {
//     user: user.reducer,
//   },
// });

// Could the resolved type be (action: string) => PayloadAction<string, string>
// instead of (...args: any[]) => PayloadAction<any, string> as calls like this work
// user.actions.setUserName(1, 'foo', true);
user.actions.setUserName('eric');

const state = store.getState();

// No autocompletion here and things like this compile and blow up at runtime:
// console.log(user.selectors.getUser2(state));
// I realize the selectorName is dynamically generated here and can make things tricky
console.log(user.selectors.getUser(state));

[Dudeonyx]

@vincentjames501 this part is wrong

const user = createSlice({
  slice: 'user',
  initialState: { name: '', age: 100 },
  reducers: {
    setUserName: (state, action: PayloadAction<string>) => {
      state.name = action.payload;
    },
  },
});

The case reducers do not receive the entire action object, just the payload. So it should be

const user = createSlice({
  slice: 'user',
  initialState: { name: '', age: 100 },
  reducers: {
    setUserName: (state, payload: string ) => {
      state.name = payload;
    },
  },
});

[markerikson]

@Dudeonyx : in RSK, they do receive the whole action:

https://github.com/reduxjs/redux-starter-kit/blob/5b38860ff4e068ee9898f9a8b6700366f04a5689/src/createReducer.ts#L56

[Dudeonyx]

@markerikson my mistake, thought it was just the payload like in robodux

[markerikson]

Yeah, I debated back and forth on that one, and decided to pass the entire action for consistency with the rest of the Redux world. Users can always destructure {payload} if they want.

[phryneas]

The problem brought up by @vincentjames501 seems to still persist - action creators are not really getting a valid payload type.
Are you still working on this or did that maybe just go unnoticed?

Edit: manually typing it works fine, but there is no real working inference of the third generic parameter. This is what it manually looks like:

const slice = createSlice<
    HomeModuleState,
    any,
    {
        increment: CaseReducer<HomeModuleState, PayloadAction<number>>;
        decrement: CaseReducer<HomeModuleState, PayloadAction<number>>;
    }
>({
    slice: 'home',
    reducers: {
        increment: (state, action: PayloadAction<number>) => ({ ...state, counter: state.counter + action.payload }),
        decrement: (state, action: PayloadAction<number>) => ({ ...state, counter: state.counter - action.payload })
    },
    initialState: { counter: 0 }
});

slice.actions.increment(5);
slice.actions.increment('foo'); // only has an error when manually typed above

[denisw]

@phryneas I created a payload-action-related fix for createSlice as #110 a while ago. Could you test if that fixes your issue?

[phryneas]

@denisw will do later, thanks :)