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

Discussion: Improve Typings #134

Closed
hedgerh opened this issue Apr 15, 2019 · 28 comments
Closed

Discussion: Improve Typings #134

hedgerh opened this issue Apr 15, 2019 · 28 comments

Comments

@hedgerh
Copy link

@hedgerh hedgerh commented Apr 15, 2019

I want to help improve the typings for redux-starter-kit and get some documentation written up around TS usage.

I have an ideal usage/API in mind, based on typesafe-actions.

Creating actions with createTypesafeAction and creating reducers with createReducer:

import { createTypesafeAction, createReducer, ActionType } from 'redux-starter-kit'

// createTypesafeAction is an optional API addition,
// but gives a cleaner way to do this than `createAction`
const actions = {
  login: createTypesafeAction('LOGIN')<ILogin>(),
  loginSuccess: createTypesafeAction('LOGIN_SUCCESS')<IUser>(),
}

interface IState {
  isLoading: boolean;
  user?: IUser;
}

const initialState: IState = {
  isLoading: false,
}

// borrowed from typesafe-actions,
// lets us get types from a map of action creators and keep type literals intact
type Action = ActionType<typeof actions>

export const reducer = createReducer<IState, Action>(initialState, {
  [actions.login.type]: (state, action) => {
    // since we have type literals in tact,
    // action should be inferred to PayloadAction<"LOGIN", ILogin>
    state.isLoading = true
  },
  [actions.loginSuccess.type]: (state, action) => {
    // action inferred to PayloadAction<"LOGIN_SUCCESS", IUser>
    state.isLoading = false
    state.user = action.payload
  },
})

I have everything working down to actually being able to infer action based on the caseReducer key. I'm hitting a bit of a limit to my skills with Typescript to infer type based on key.

createSlice

I use redux-saga. With typesafe-actions, I'm able to type the action parameter in my sagas by doing:

const actions = { login: createStandardAction('LOGIN')<ILogin>() }

function* loginSaga(action: ActionType<typeof actions.login>) {}

I'm not sure how you'd go about doing something like this with createSlice. I'm trying to think of some way that you could type the action param in your case reducers, and also come out with actions that have the correct literals for type and the correct payload shapes.

Would love to hear other people's thoughts on how we could improve the typings.

@markerikson

This comment has been minimized.

Copy link
Collaborator

@markerikson markerikson commented Apr 15, 2019

Obligatory paging of the TS Experts: @denisw, @Dudeonyx , @Jessidhia

@tim-hm

This comment has been minimized.

Copy link

@tim-hm tim-hm commented Apr 15, 2019

I've been trying to find the optimal way here as well .... What follows is my current approach which works well bc I get type checking on actions, state, and effects (thunks). I would love to have someone tell me a better way! However, I don't think we can reduce the boilerplate much w/o compromising elsewhere. A few preliminaries to my setup:

a. For each slice I follow the duck folder pattern.
b. Every folder in my project has an index.ts file whose reason d'etre is to export/import stuff or as the applications entry point.
c. For every view I have a file named as the component home.tsx, an index.ts which exports it, and if it needs any higher order stuff (i.e., access to the store) then a connect.ts so my view remains decoupled (pure), and easy to test.

… So a slice then:

src/store/slices/foo/index.ts:

export * from "./foo"

src/store/slices/foo/foo.ts:

import { createSlice } from "redux-starter-kit"
import * as reducers from "./reducers"
import * as effects from "./effects"
import { INITIAL_STATE } from "./constants"

const slice = createSlice({
  initialState: INITIAL_STATE,
  reducers,
  slice: "foo",
})

export const foo = {
  ...slice,
  effects, // Effects to me are thunks, but i keep them with this slice to everything is neatly packaged together for consumption elsewhere in my app
}

src/store/slices/foo/constants.ts

import { IFooState } from "src/typings" // IRootState and ISliceStates in here

export const INITIAL_STATE: IFooState = {
  foo: "bar"
}

src/store/slices/foo/reducers.ts

import { IFooState, IAction } from "src/typings" // IAction only has type: string and payload: any in it

interface ISetFooAction extends IAction {
  payload: string
}

export const setFoo = (state: IFooState, action: ISetFooAction) => {
  _.set(state, "foo", action.payload)
}

… Somewhere else in my view logic

src/views/foo/index.ts

export * from "Foo"

src/views/foo/foo.tsx

...
interface IProps {
  foo: string
  setFoo(value: string): void  
}

export const Foo: React.FC<IProps> => ({ foo, setFoo }) => (
	<div>
  	<button onClick={() => setFoo("no more bar!")} />
		<h6>{foo}</h6>
  </div>
)

src/views/foo/connect.ts

import { connect } from "react-redux"
import { IRootState, IFooState } from "src/store/typings"
import { foo } from "src/store"
import { Foo } from "./foo"

const mapState = (state: IRootState) => {
  const { foo } = state.foo
  return {
    foo,
  }
}

const { setFoo } = foo.actions

const mapDispatch = {
  setFoo: (value: string) => setFoo(value),
}

const Connected = connect(
  mapState,
  mapDispatch
)(Foo)
export { Connected as Foo }

Apologies for mistakes above (from memory). Let me know your thoughts.

@hedgerh

This comment has been minimized.

Copy link
Author

@hedgerh hedgerh commented Apr 15, 2019

Yeah, I'm wondering if that's about as clean as we're gonna get with createSlice.

The thing that typesafe-actions does that redux-starter-kit currently doesn't is preserve the literal type of type on each action. So your ISetFooAction interface is shaped { type: string, payload: string } instead of { type: 'SET_FOO', payload: string }. When you have your actions more strongly typed like that, Typescript can start making inferences about the shape of your payload that are pretty handy if it recognizes the type.

Regardless, I think we should try to figure out the best implementation we can and start writing some docs around it.

@tim-hm

This comment has been minimized.

Copy link

@tim-hm tim-hm commented Apr 16, 2019

When you have your actions more strongly typed like that, Typescript can start making inferences about the shape of your payload that are pretty handy if it recognizes the type.

Could you elaborate on this? I don't follow on why the literally typed type field makes a difference when its shape is explicitly declared by ISetFooAction. Or are you saying that it removes the need for ISetFooAction altogether?

@hedgerh

This comment has been minimized.

Copy link
Author

@hedgerh hedgerh commented Apr 16, 2019

Sorry, big copy and paste, but take a look at how I do ducks with typesafe-actions

import { call, put, takeLatest } from 'redux-saga/effects'
import { createStandardAction, ActionType, getType } from 'typesafe-actions'
import client from '../httpClient'

// TYPES
interface ILogin {
  email: string;
  password: string;
}

interface IUser {
  id: number;
  username: string;
}

// HTTP
const http = {
  login: (credentials: ILogin) => client.post('/login', credentials),
}

// ACTION CREATORS
const actions = {
  login: createStandardAction('LOGIN')<ILogin>(),
  loginSuccess: createStandardAction('LOGIN_SUCCESS')<IUser>(),
  loginError: createStandardAction('LOGIN_ERROR')<string>(),
}

type LoginAction = ActionType<typeof actions.login>

// SAGAS
function* loginSaga(action: LoginAction) {
  try {
    const user: IUser = yield call(http.login, action.payload)
    yield put(actions.loginSuccess(user))
  } catch (e) {
    yield put(actions.loginError('Uh oh'))
  }
}

export function* authSagas() {
  yield takeLatest('LOGIN', loginSaga)
}

// STATE
interface IState {
  isLoading: boolean;
  user?: IUser;
}

const initialState: IState = {
  isLoading: false,
}

type Action = ActionType<typeof actions>
                              
// REDUCER
export default function reducer(state: IState, action: Action) {
  switch (action.type) {
    case actions.login):
      return { ...state, isLoading: true }
    case getType(actions.loginSuccess):
      return { ...state, user: action.payload }
    case getType(actions.loginError):
      return { ...state, isLoading: false }
  }
  return state
}

so when I create my actions:

// ACTION CREATORS
const actions = {
  login: createStandardAction('LOGIN')<ILogin>(),
  loginSuccess: createStandardAction('LOGIN_SUCCESS')<IUser>(),
  loginError: createStandardAction('LOGIN_ERROR')<string>(),
}

my action end up with this kind of type: PayloadAC<"LOGIN", ILogin> for the my login one. they all retain their literal action type, instead of just string.

I can then do this:

type Action = ActionType<typeof actions>
// PayloadAction<"LOGIN", ILogin> | PayloadAction<"LOGIN_SUCCESS", IUser> | PayloadAction<"LOGIN_ERROR", string>

Now, in my reducer, within each case statement, Typescript knows exactly which action it is because it compares the action.type, so I get the correct type completions and type safety scoped within each of my case statements. Make sense?

// REDUCER
export default function reducer(state: IState, action: Action) {
  switch (action.type) {
    case actions.login.type:
      return { ...state, isLoading: true }
    case actions.loginSuccess.type:
      return { ...state, user: action.payload } // it knows `action.payload` is type `IUser` here
    case actions.loginError.type:
      return { ...state, isLoading: false }
  }
  return state
}

So to answer your question, that's kind of what I'd like to get to. If you could just use a generic argument somewhere to assert the payload shape, maybe it could match up the action type to the caseReducer key. I don't know, though. This stuff is a bit over my head as far as implementation or whether that's actually possible, though.

@tim-hm

This comment has been minimized.

Copy link

@tim-hm tim-hm commented Apr 16, 2019

Makes sense! I see that we're structuring slices differently. You use the literal type so that your reducer can infer the typing and letting typesafe-actions link it together. But since I'm using createSlice and I'm explicitly declaring the action type for each reducer function, I don't need those steps.

I prefer separating reducer cases because I find them easier to test/read and then let RSK take care of wiring it up ... otherwise whats the point of using RSK? Additionally, I don't use anything from the typesafe-actions but still get the same level of typing because each reducer case is explicitly. typed.

I don't see an advantage to one approach or the other but it would be nice if RSK had a preferred and documented/supported approach.

@hedgerh

This comment has been minimized.

Copy link
Author

@hedgerh hedgerh commented Apr 16, 2019

To be clear, I'm not using RSK at all in my code above, if you didn't realize. I don't know if there are too many other ways to type the createSlice() approach, at this point. the only thing would be if you defined all of your case reducers when you are calling createSlice, you could probably pass it a generic argument to type all of your state params, maybe? createSlice<State>(...)

I wanna see if anyone else chimes in, but then I'd like to start documenting.

@tim-hm

This comment has been minimized.

Copy link

@tim-hm tim-hm commented Apr 16, 2019

I prefer separating types from logic bc it feels cleaner and more explicit vs. using <T> and type to extract type information. I also think it leads to friendlier syntax ... or perhaps I'm just scared of generic typings! Even if the option was there to go createSlice<State>(...) I'd still prefer defining the root state and individual slice structure centrally bc I think it better achieves high cohesion and low coupling.

@Dudeonyx

This comment has been minimized.

Copy link
Contributor

@Dudeonyx Dudeonyx commented Apr 17, 2019

@hedgerh I was working on a prototype of sorts that might be what you're looking for, at least for createSlice.
But bear in mind that due to the limitations of typescript regarding concatenated strings there is no way to do this automatically since our action types in createSlice are namespaced, so due to the manual typing by the end user the possibility of user mistakes exists.

Before that I should point out that you can use the included createAction utility to create type safe actions already in case you didn't know, i.e

import { IDetials } from './types';

const actions ={
     updateDetails: createAction<IDetials, 'UPDATE_DETAILS'>('UPDATE_DETAILS')
};

const bla = createReducer({
    reducers: {
        [actions.updateDetails.type]: (state, action)=>{
        // ....
        }
    }
})

Anyway back to the createSlice stuff
it would work like this

  const { actions: { setName, setSurname, setMiddlename, reset } } = createSlice({
    slice: 'form',
    cases: {
      setName: (state, action: PayloadAction<string, 'form/setName'>) => {
        state.name = action.payload;
      },
      setSurname: (state, action: PayloadAction<string,'form/setSurname'>) => {
        state.surname = action.payload;
      },
      setMiddlename: (state, action: PayloadAction<string,'form/setMiddlename'>) => {
        state.middlename = action.payload;
      },
      resetForm: (state, _: PayloadAction<undefined, 'form/resetForm'>) => formInitialState,
    },
    initialState: formInitialState,
  });

This creates fully typed action creators whose type and toString() properties return string literals.

image

image

image

E: the slice property is something I'm thinking of proposing as a means to know what slice an action creator belongs to.

@hedgerh

This comment has been minimized.

Copy link
Author

@hedgerh hedgerh commented Apr 17, 2019

Awesome! This is very close to my dream syntax. This lets me type the action my saga takes as an argument the way that I'd prefer, where it's directly tied to the actual action.

Any possible solves to make the end user not have to manually type? Is getting typescript to play nice with concatenation a complete no go, or should we explore it more? Otherwise, are there other workaround? Also, suppose concatenation weren't an issue, what could it look like then? How would the end user type the payload shape?

I'd love to see your work on this, btw! Do you have a fork/branch up somewhere?

And yeah, I knew you could get literal typed actions with createAction. I was thinking about proposing something like createTypedAction that would use typesafe-action's approach that lets you type the payload without double passing the action type: createTypedAction('LOGIN')<ILogin>().

Any idea how we could get createReducer to infer the shape of the action argument in each case reducer? Like this:

const actions = {
  login: createAction<ILogin, 'LOGIN'>('LOGIN'),
  loginSuccess: createAction<IUser, 'LOGIN_SUCCESS'>('LOGIN_SUCCESS'),
}

type Action = ActionType<typeof actions> // approach borrowed from typesafe-actions

const reducer = createReducer<State, Action>({
  [actions.login.type]: (state, action) => {
    state.user = action.payload // would blowup bc payload shape isn't IUser
  },
  [actions.loginSuccess.type]: (state, action) => {
    state.user = action.payload // good! typescript infers that action.payload is IUser
  }
})
@denisw

This comment has been minimized.

Copy link
Contributor

@denisw denisw commented Apr 17, 2019

Is getting typescript to play nice with concatenation a complete no go, or should we explore it more?

I think it is impossible. It would require some kind of string concatenation operator in the TypeScript type system, which doesn't exist as far as I know. Therefore, the current design of createSlice doesn't let us infer any action type that is more specific than string.

Any idea how we could get createReducer to infer the shape of the action argument in each case reducer?

Below is the best I've come up with so far:

export type Actions<T extends keyof any = string> = {
  [K in T]: PayloadAction<any>
}

export type CaseReducers<S, AS extends Actions> = {
  [T in keyof AS]: CaseReducer<S, AS[T]>
}

export function createReducer<S, AS extends Actions<any> = Actions<any>>(
  initialState: S,
  actionsMap: CaseReducers<S, AS>
): Reducer<S> {
  // ...
}

That kinda works as in the example snippet, but is limited to the kinds of payload actions generated by createAction. If I change PayloadAction<any> to Action<any>, code like this:

type CounterAction =
  | { type: 'increment'; payload: number }
  | { type: 'decrement'; payload: number }

const incrementHandler = (state: number, action: CounterAction) => state + 1
const decrementHandler = (state: number, action: CounterAction) => state - 1

const reducer = createReducer(0 as number, {
  increment: (state, action) => state + action.payload,
  decrement: (state, action) => state + action.payload
})

gives me errors like:

Type '(state: number, action: CounterAction) => number' is not assignable to type 'CaseReducer<number, AnyShapeAction<string>>'.
  Types of parameters 'action' and 'action' are incompatible.
    Type 'AnyShapeAction<string>' is not assignable to type 'CounterAction'.
      Type 'AnyShapeAction<string>' is not assignable to type '{ type: "decrement"; payload: number; }'.
@denisw

This comment has been minimized.

Copy link
Contributor

@denisw denisw commented Apr 17, 2019

P.S.: In the Redux Preboiled library I recently released, I chose a somewhat different API design which makes typing much easier, and might serve as inspiration for RSK.

import {
  chainReducers,
  createAction,
  onAction,
  withInitialState
} from 'redux-preboiled'

const increment = createAction('increment')
const multiply = createAction('multiply').withPayload<number>()
​
const counterReducer = chainReducers(
  withInitialState(0),
  onAction(increment, state => state + 1),
  onAction(multiply, (state, action) => state * action.payload)
)

Each onAction call creates a case reducer whose action parameter type is automatically inferred from the action creator passed as the first argument. Becaue each onAction call is type-inferred separately, the complexity around type inference from object keys. Perhaps such a design is also a good fit for Starter Kit.

@markerikson

This comment has been minimized.

Copy link
Collaborator

@markerikson markerikson commented Apr 17, 2019

FWIW, I do particularly like the current "object keys" definition approach. I'm certainly interested in any reasonable ways that the RSK TS typings can be improved, but if it comes down to reworking the API to get "perfect" TS behavior, vs keeping the same API and only getting "adequate" TS behavior, I'm going to opt for the latter.

I would say my general priorities are:

  1. Overall API shape
  2. Ease of use from JS
  3. Overall TS compatibility
  4. Ease of use from TS
  5. TS "correctness"
@hedgerh

This comment has been minimized.

Copy link
Author

@hedgerh hedgerh commented Apr 18, 2019

I agree with @markerikson . Hopefully we can figure out how to get things to where it's a win-win for both JS and TS users.

@denisw I like the withPayload api you came up with, but that method would be meaningless to non-Typescript users, so it could create some confusion about what it does, exactly.

Mark, where do you stand as far as, rather than reworking the API, potentially making additions to the API to support Typescript users?

For example, adding something like createTypedAction that gives Typescript users a good API for creating actions the way they'd like?

PS. My motivation for wanting to use RSK: I am generally anti-abstracting Redux code, but I'd be very interested in adopting RSK, since the abstractions would have a more official "blessing." My reason being that new developers on my project shouldn't have to learn some abstractions that I either wrote or pulled in from some library that isn't ubiquitous in the community.

Abstractions aside, I currently need to pull in a library that doesn't have official support to get really great type safety around my normal Redux code. It'd be great if redux-starter-kit or something else official could be the solve for this.

@markerikson

This comment has been minimized.

Copy link
Collaborator

@markerikson markerikson commented Apr 19, 2019

I'm a bit hesitant to add an API that only supports TS usage. Also, how would using that look with createSlice(), if at all?

@hedgerh

This comment has been minimized.

Copy link
Author

@hedgerh hedgerh commented Apr 19, 2019

I just wanted to see where you stood on potentially adding stuff that was geared towards TS usage, if it became necessary.

The one place where it may make sense is with doing something like the API from Redux Preboiled:

const multiply = createAction('multiply').withPayloadType<number>()

It's possible to get the job done with createAction, but that looks like:

const multiply = createAction<number, 'multiply'>('multiply')

Not ideal, as you need to write the actual type string twice. You could create a constant and re-use it in both spots, but the point of all of this is to cut down on boilerplate, right?

Alternative would be to have something like createTypedAction that had the API

const multiply = createTypedAction('multiply')<number>()

Either way, it wouldn't be unusable by a non-typescript user, but there'd be no point. From a JS perspective,

createAction().withPayloadType() or createTypedAction()() should return an action creator to a non-ts user just the same.

@denisw

This comment has been minimized.

Copy link
Contributor

@denisw denisw commented Apr 19, 2019

@denisw I like the withPayload api you came up with, but that method would be meaningless to non-Typescript users, so it could create some confusion about what it does, exactly.

To clarify, in Preboiled the withPayload() part is meaningful in JavaScript as well. This is because in Preboiled, the withPayload() call is required if you want the action creator to take a payload. Plain createAction() creates an parameter-less action creator instead.

Mark, where do you stand as far as, rather than reworking the API, potentially making additions to the API to support Typescript users?

For example, adding something like createTypedAction that gives Typescript users a good API for creating actions the way they'd like?

I agree with @markerikson that API which makes only sense for TypeScript is a bit awkward.

PS. My motivation for wanting to use RSK: I am generally anti-abstracting Redux code, but I'd be very interested in adopting RSK, since the abstractions would have a more official "blessing."

To add to that, the nice thing about most of Redux Starter Kit's helpers is that they don't add new abstractions, but just provide boilerplate for existing ones (action creators and reducers). createSlice is the one exception here, and one you don't have to use if it's not your cup of tea (it's not for me, for instance).

@markerikson

This comment has been minimized.

Copy link
Collaborator

@markerikson markerikson commented Apr 28, 2019

Is there anything reasonably actionable that can be done based on this discussion?

As I said, I'm open to further typings tweaks if they improve things, but I'm not looking to meaningfully change the existing APIs.

@denisw

This comment has been minimized.

Copy link
Contributor

@denisw denisw commented Apr 28, 2019

Without significant API changes, there is nothing major that could be done. I think it would make most sense to close this and open more specific issues as they arise.

@hedgerh

This comment has been minimized.

Copy link
Author

@hedgerh hedgerh commented Apr 28, 2019

I'm interested in @Dudeonyx's work on adding better type safety around createSlice. Other than that, adding a usage guide for typescript users would be a nice action item to come from this.

@Dudeonyx

This comment has been minimized.

Copy link
Contributor

@Dudeonyx Dudeonyx commented Apr 28, 2019

@hedgerh Sorry, I actually finished working on it a while ago but I forgot to adapt it to redux-starter-kit and make a pull request, doing so now

@Dudeonyx

This comment has been minimized.

Copy link
Contributor

@Dudeonyx Dudeonyx commented Apr 29, 2019

PR is up

@hedgerh

This comment has been minimized.

Copy link
Author

@hedgerh hedgerh commented Apr 29, 2019

Aside from the createSlice PR that's currently open, what do we recommend as far as typescript usage, so we can start to work on some kind of documentation?

I still think it'd be great if we found some way to infer the types of actions inside of createReducer. I may mess around with this a bit and see if I can come up with anything. Not sure if @Dudeonyx has any ideas there.

@markerikson

This comment has been minimized.

Copy link
Collaborator

@markerikson markerikson commented Apr 29, 2019

I'll defer to y'alls expertise on that topic. If you can come up with recommendations, I will gladly accept a "Usage with TS" docs page PR.

@jashworth

This comment has been minimized.

Copy link

@jashworth jashworth commented May 8, 2019

I couldn't find a full example, so I tried writing one myself that includes redux-observable.

It borrows the withPayload idea from denis and uses Martin Hochel's rex-utils.

https://codesandbox.io/s/ywxzxzk24v

For me, the ability to narrow actions to a specific type via a discriminated union (e.g. ofType in epics.ts) outweighs the convenience of createSlice.

@jaysoo

This comment has been minimized.

Copy link

@jaysoo jaysoo commented Aug 1, 2019

Adding to the discussion. There are some soundness problems when using actions from a slice outside of its reducers.

Here are some examples to show what's working as expected and what's not.

import { createSlice, PayloadAction } from 'redux-starter-kit';

const a = createSlice({
  initialState: { message: '' },
  reducers: {
    saySomething: (state, action: PayloadAction<string>) => {
      // OK: This works because payload type matches state.
      state.message = action.payload;

      // OK: Type mistmatch
      const x: number = action.payload;
    }
  }
});

// OK: Type mistmatch
a.actions.saySomething(1);

// Use the a's action in extra reducer
const b = createSlice({
  initialState: {},
  reducers: {},
  extraReducers: {
    [a.actions.saySomething.type]: (state, action) => {
      // PROBLEM: Passes but is incorrect because the payload is a `string`
      const x: number = action.payload;
    }
  }
});


// Use the a's action in extra reducer
const c = createSlice({
  initialState: {},
  reducers: {},
  extraReducers: {
    [a.actions.saySomething.type]: (state, action: ReturnType<typeof a.actions.saySomething>) => {
      // OK: Type mismatch
      const x: number = action.payload;
    }
  }
});
@markerikson

This comment has been minimized.

Copy link
Collaborator

@markerikson markerikson commented Oct 13, 2019

@jaysoo : yeah, given the disconnect in declarations, I'm not particularly sure how that would be fixable.

@markerikson

This comment has been minimized.

Copy link
Collaborator

@markerikson markerikson commented Oct 22, 2019

I'm going to call this one resolved thanks to the recent changes in 0.7 and 0.8. Thanks for all the tweaks and suggestions!

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