reduxjs / rtk-github-issues-example

Source code for the Redux Toolkit Advanced Tutorial
214 stars 97 forks source link

Type signature of actions created by slices #7

Closed RobertMenke closed 5 years ago

RobertMenke commented 5 years ago

Hey, I'm curious why the type definitions for the action creators we get back from createSlice expects 2 parameters when the action creators are only ever called with a single parameter.

Screen Shot 2019-11-19 at 10 14 48 PM

I see why it works, but would it make sense to update the type definition to use the signature that you're using in the docs?

markerikson commented 5 years ago

That doesn't sound right. Can you post the code that generated the displayRepo action creator?

Erm. Wait, this is on the example repo.

Where are you seeing that issue, specifically? Is it just from cloning the repo, in one of the sandboxes, or something else?

FWIW, if I open the repo as of its final commit and hover over displayRepo, I don't see any errors, and I see this type signature:

image

RobertMenke commented 5 years ago

Hey, this is from cloning the repo and then opening it up in IntelliJ. After clicking into the function IntelliJ does seem to think that the type definition should match

    displayRepo(state, action: PayloadAction<CurrentRepo>) {
      const { org, repo } = action.payload
      state.org = org
      state.repo = repo
    },

After looking into the types a bit more I believe this actually may be an IntelliJ bug.

declare type CaseReducerActions<CaseReducers extends SliceCaseReducerDefinitions<any, any>> = {
    [Type in keyof CaseReducers]: IfIsCaseReducerWithPrepare<CaseReducers[Type], ActionCreatorWithPreparedPayload<PrepareActionForReducer<CaseReducers[Type]>>, IfIsReducerFunctionWithoutAction<CaseReducers[Type], ActionCreatorWithoutPayload, PayloadActionCreator<PayloadForReducer<CaseReducers[Type]>>>>;
};

^^ The above doesn't seem to have any reference to state as an expected parameter, although the types are quite hard to parse at first glance :).

markerikson commented 5 years ago

The above doesn't seem to have any reference to state as an expected parameter, although the types are quite hard to parse at first glance :).

Hah, no kidding :) I'm the maintainer, and I barely understand them! :) I'm grateful that there's some TS experts in the Redux community who have been able to put those together for us.

The action creators definitely do not have any references to the state, either as an argument or a related type. They do reuse the type of the action argument for the reducer. For this example, since action is a PayloadAction<CurrentRepo>, the action creator knows that it should return {type: "issuesDisplay/displayRepo, payload: CurrentRepo}. It also will have a signature of (payload: CurrentRepo) => PayloadAction<CurrentRepo>.

markerikson commented 5 years ago

FWIW, I just opened up that same project in WebStorm 2019.1, and the type appears correct there too: waitasec what is that?

image

No. No, there's definitely not supposed to be a state argument for displayRepo. Wut.

I wonder if it's somehow mixing up the action creators and the reducers? Especially since it's labeling the arguments (state, action), and not (payload) like the action creator should have.

RobertMenke commented 5 years ago

Hmm yes looks like a Jetbrains bug... sigh I don’t think they use LSP for typescript. Thank you for looking into this one! I may open an issue in their issue tracker and reference this issue.

markerikson commented 5 years ago

Yeah, I was just about to do that, actually :)

markerikson commented 5 years ago

Went ahead and filed https://youtrack.jetbrains.com/issue/WEB-42559 .

I'll go ahead and close the issue, since this doesn't appear to be a problem in RTK itself. Thanks for reporting it, though!

luokuning commented 4 years ago

Went ahead and filed https://youtrack.jetbrains.com/issue/WEB-42559 .

I'll go ahead and close the issue, since this doesn't appear to be a problem in RTK itself. Thanks for reporting it, though!

I just vote for the request on youtrack.