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

Improve typings: Added ability to typecast action types in createSlice #136

Closed
wants to merge 11 commits into from

Conversation

Dudeonyx
Copy link
Contributor

@Dudeonyx Dudeonyx commented Apr 29, 2019

Referencing issue #134 about improving typings while not changing the core api just for typescript users.

This PR adds the ability to typecast action types in createSlice, works as follows

const formSlice = createSlice({
  slice: 'form',
  reducers: {
    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,
});

giving the following action creator type signatures in formSlice.actions

image

image

image

Note: payload types undefined, never , and void have action creator signature types requiring no payloads
image

Caveat: Possibility of user error in typecasting exists but until typescript supports string literal concatenation not much can be done.

Bonus: I 'fixed' the typescript issue in createReducer, so no more need to // @ts-ignore that part, Fix has no impact on exposed api's or types.

Bonus 2: the slice argument and return in createSlice is now a string literal, hooray for slightly barely better type safety

actionCreators implementation and type signature
added String literal type param to createSlice type signature for typing slice.
changed `CaseReducerActionPayloads` type to `CaseReducerActions` to extract entire action.
No foreseeable benefits and thus just technical debt
@netlify
Copy link

netlify bot commented Apr 29, 2019

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

Built with commit 3562ff2

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

so payload types `undefined`, `never`  and  `void`
have action creator type signatures requiring no payload
@Dudeonyx
Copy link
Contributor Author

Dudeonyx commented Apr 29, 2019

Pinging @denisw, @Jessidhia, @hedgerh, @markerikson

@hedgerh
Copy link

hedgerh commented Apr 29, 2019

Will take a look when I get home later 👀

@markerikson
Copy link
Collaborator

For the still-mostly-TS-clueless: can I get a comparison of both the general usage and the type-checking status before and after this PR? What do things look like now, and what do they look like as a result of this?

Handling `never` is not worth the complication
@Dudeonyx
Copy link
Contributor Author

Dudeonyx commented Apr 29, 2019

For the still-mostly-TS-clueless: can I get a comparison of both the general usage and the type-checking status before and after this PR? What do things look like now, and what do they look like as a result of this?

@markerikson The changes introduced are not breaking for TS users, in fact unless told about it most people won't notice it.

Previously you would typecast your reducers actions like this

const formSlice = createSlice({
  slice: 'form',
  reducers: {
    setName: (state, action: PayloadAction<string>) => {
      state.name = action.payload;
    },
  },
});

formSlice.actions.setName would have the following type signature:

image

With this PR, first of all, the type prop in the action creator is exposed in the type signature, becoming like this:

image

Second, users can now optionally typecast the action type to a string literal instead of a generic string i.e

const formSlice = createSlice({
  slice: 'form',
  reducers: {                          // Talking about \|/
    setName: (state, action: PayloadAction<string, 'form/setName'>) => {
      state.name = action.payload;
    },
  },
});

So formSlice.actions.setName's type signature becomes

image

Notice that the type prop is no longer just a generic string but now a specific string literal form/setName

@hedgerh
Copy link

hedgerh commented Apr 29, 2019

What happens if you intentionally misspell the action type in the case reducer args annotation? Sorry didn’t have a chance to look at this last night. About to take a look

@hedgerh
Copy link

hedgerh commented Apr 29, 2019

What does current usage look like? when creating a slice, do you just assert action: PayloadAction<PayloadShape>?

Second, users can now optionally typecast the action type to a string literal instead of a generic string

So we don't lose the ability to assert the shape of just the payload if we aren't concerned with the action type, correct?

@markerikson this is useful for someone who needs to make use of these actions elsewhere and doesn't want to create a whole new interface for it. I would use this like so:

const { actions, reducer } = createSlice({
  slice: 'auth',
  reducers: {
    login: (state, action: PayloadAction<ILogin, 'auth/login'>) => ...
  }
})

export function* loginSaga (action: ActionType<typeof actions.login>) {
  // rather than creating some extra interface that isn't strongly tied to the actual action,
  // i can type the saga's arg based on the actual action I'm using.
}

export function* watchSaga () {
  yield takeLatest(actions.login.type, loginSaga)
}

@Dudeonyx we may need some ActionType type that can be used to type actions by the action's type. agree? If not, is there another way to do this without one? Going off what typesafe-actions does.

Also, do we need to align with #109 at all? @denisw

@markerikson
Copy link
Collaborator

I need some advice from you TS experts on this one. You think it's worth merging / ready to merge?

@hedgerh @Dudeonyx @denisw @Jessidhia

@hedgerh
Copy link

hedgerh commented May 20, 2019

Hopefully @Dudeonyx can get back about my last comment. I'll pull this down tonight and see how it's looking, though.

@markerikson
Copy link
Collaborator

I'd still appreciate some further feedback on this PR.

@markerikson
Copy link
Collaborator

Following up again. Is this still relevant after all the other recent changes?

@phryneas
Copy link
Member

phryneas commented Aug 3, 2019

This is still relevant, as the recent changes did not affect the type of an action - but given all the recent changes, it would have to be rewritten. (Also, this PR contains quite a few commits not directly correlating with the PR itself, so those might be better off in separate PRs?)

It still bears the problem of user-specified types potentially not matching the real, generated type. But as TS has no type-string-concatenation, this is the best way to currently do this and TS cannot check those. And at least, an error would be located better directly in the slice, close to the slice name definition and the reducer's name than somewhere else. Potential errors would be more easy to spot that way.

PS: as already mentioned, doing it this way works only for reducers that actually specify their action. This could be solved by adding a type PlainResolver<ActionType extends string> and casting all those reducers to that type.

@markerikson
Copy link
Collaborator

@Dudeonyx, @phryneas : can someone rebase / reimplement this on latest master?

@markerikson
Copy link
Collaborator

Well, this has been sitting around for months. I'll continue to leave it open for now, but unless someone can take the time to fix up the conflicts, I don't see it getting into 0.8 or 1.0.

@markerikson
Copy link
Collaborator

Actually, looking at the current source, is this already covered?

export type PayloadAction<
  P = any,
  T extends string = string,
  M = void
> = WithOptionalMeta<M, WithPayload<P, Action<T>>>

Meh. I'm going to go ahead and close this. If someone can take the time to review the current behavior in comparison, I'm still open to trying to improve it if possible.

@markerikson markerikson closed this Oct 7, 2019
markerikson pushed a commit that referenced this pull request Apr 20, 2021
* Use code-generation everywhere
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants