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

TypeScript rewrite #73

Merged
merged 38 commits into from Jan 19, 2019
Merged

TypeScript rewrite #73

merged 38 commits into from Jan 19, 2019

Conversation

@denisw
Copy link
Contributor

denisw commented Jan 7, 2019

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

This comment has been minimized.

Copy link

netlify bot commented Jan 8, 2019

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

Built with commit 497babf

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

@denisw denisw changed the title WIP: Typescript rewrite TypeScript rewrite Jan 8, 2019
@denisw

This comment has been minimized.

Copy link
Contributor Author

denisw commented Jan 8, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

denisw commented Jan 8, 2019

@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.

@denisw denisw mentioned this pull request Jan 10, 2019
tsconfig.json Show resolved Hide resolved
Copy link

resir014 left a comment

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

.babelrc Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
@denisw denisw force-pushed the denisw:typescript branch from 60a9ddc to e3b2b27 Jan 12, 2019
Copy link

Jessidhia left a comment

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.

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
rollup.config.js Outdated Show resolved Hide resolved
*/
export function getDefaultMiddleware(
isProduction = IS_PRODUCTION
): Middleware[] {

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Jan 12, 2019

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

This comment has been minimized.

Copy link
@Dudeonyx

Dudeonyx Jan 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@denisw

denisw Jan 12, 2019

Author Contributor

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 & ...?

This comment has been minimized.

Copy link
@Dudeonyx

Dudeonyx Jan 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@denisw

denisw Jan 12, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@denisw

denisw Jan 13, 2019

Author Contributor

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

src/configureStore.ts Show resolved Hide resolved
src/serializableStateInvariantMiddleware.ts Outdated Show resolved Hide resolved
src/sliceSelector.ts Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
@denisw

This comment has been minimized.

Copy link
Contributor Author

denisw commented Jan 12, 2019

@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.

@denisw denisw force-pushed the denisw:typescript branch from e3b2b27 to 1bbcae6 Jan 12, 2019
src/configureStore.ts Outdated Show resolved Hide resolved
denisw added 18 commits Jan 7, 2019
This is a pre-requisite to using @babel/preset-typescript later.

This change required upgrading rollup-plugin-babel, which in turn
required upgrading Rollup.js itself. Fortunately, the upgraded
rollup-plugin-babel automatically takes care of Babel helpers
deduplication and turning off module transpilation in Babel's env
preset, so the Babel configuration could be drastically simplified.
For the same reason, we don't need rollup-plugin-replace anymore.
It's apparently not strictly needed for Babel's TypeScript plugin,
but at least more explicit.
This allows more concrete types like `Action<'increment'>` to
be inferred by TypeScript.
@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jan 15, 2019

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!

denisw added 2 commits Jan 14, 2019
For now, we need to use the Git version of typescript-eslint-parser
because the last release only supports TypeScript 3.1. A new release
is planned for any day now, so we should be able to move to an NPM
release soon.

eslint/typescript-eslint-parser#596

As ESLint's no-unused-vars doesn't work well with TypeScript, I
enabled noUnusedLocals and noUnusedParameters on the TypeScript
side instead.
@phryneas

This comment has been minimized.

Copy link
Collaborator

phryneas commented Jan 15, 2019

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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jan 15, 2019

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

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jan 16, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

denisw commented Jan 16, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

denisw commented Jan 16, 2019

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

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jan 17, 2019

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

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jan 19, 2019

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

@markerikson markerikson merged commit 0e139e9 into reduxjs:master Jan 19, 2019
7 checks passed
7 checks passed
Header rules - redux-starter-kit-docs No header rules processed
Details
Pages changed - redux-starter-kit-docs All files already uploaded
Details
Redirect rules - redux-starter-kit-docs No redirect rules processed
Details
Mixed content - redux-starter-kit-docs No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
netlify/redux-starter-kit-docs/deploy-preview Deploy preview ready!
Details
This was referenced Jan 19, 2019
@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jan 19, 2019

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

@jerolimov

This comment has been minimized.

Copy link
Contributor

jerolimov commented Jan 19, 2019

It works! Thanks @denisw and @markerikson

@vincentjames501

This comment has been minimized.

Copy link

vincentjames501 commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Contributor

Dudeonyx commented Jan 23, 2019

@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

This comment has been minimized.

@Dudeonyx

This comment has been minimized.

Copy link
Contributor

Dudeonyx commented Jan 23, 2019

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

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Jan 23, 2019

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

This comment has been minimized.

Copy link
Collaborator

phryneas commented Feb 13, 2019

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

This comment has been minimized.

Copy link
Contributor Author

denisw commented Feb 14, 2019

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

@phryneas

This comment has been minimized.

Copy link
Collaborator

phryneas commented Feb 15, 2019

@denisw will do later, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.