reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.71k stars 1.17k forks source link

Default reducer for slice #311

Closed negebauer closed 4 years ago

negebauer commented 4 years ago

Currently it seems there's no way to add a default reducer to a slice. This is useful when using libraries like normalizr with an entities slice. The entities slice listens for all actions and then checks for the presence of entities in the payload.

Currently I had to create an old fashioned reducer for using normalizr. It would be useful if createSlice offers a defaultReducer or something like that that runs for all actions.

This is what I'm currently using

import merge from 'lodash/merge'
import produce from 'immer'

const entitiesSliceName = 'entities'

function entitiesReducer(baseState = {}, { payload }) {
  if (!payload || !payload.entities) return baseState

  return produce(baseState, draftState => {
    merge(draftState, payload.entities)
  })
}

export default {
  reducer: entitiesReducer,
  name: entitiesSliceName,
}

This is what I think could be usefull

import { createSlice } from '@reduxjs/toolkit'
import merge from 'lodash/merge'

const entitiesSlice = createSlice({
  name: 'entities',
  initialState: 0,
  defaultReducer: (state, { payload = {} }) => merge(state, payload.entities),
})

export default entitiesSlice
phryneas commented 4 years ago

So am understanding it correctly that you want a reducer that has no specific actions assigned to it and handles each and every action?

If that's the case: this is a custom reducer. It's just a completely different usecase than createSlice - there's no need to use createSlice everywhere. No reason to use a screwdriver for a nail I guess?

markerikson commented 4 years ago

I understand why and what you're asking for, and I don't think this is something I want to add.

Yes, the lack of a default case equivalent is a bit of a difference vs a switch statement. However, at that point you're really not even dealing with something that's specific to this slice at all.

I think you're better off having a higher-order reducer that would handle the existence of action.payload.entities, and use that to wrap the generated slice reducer, like:

export default mergeEntities(slice.reducer);

For RTK specifically, adding this would be difficult for us. Not only are createSlice's TS types already pretty complex, but createSlice is built around createReducer, which explicitly takes a lookup table of action types to case reducers, and returns the existing state if no action type matches.

I don't see enough benefit here to warrant trying to figure out how to rework things to support that use case.

negebauer commented 4 years ago

Given the problems that you mentioned with doing this I agree that it doesn’t warrant supporting that use case.

Thanks for the good and detailed answer 😊

phryneas commented 4 years ago

@markerikson I wouldn't even be opposed to add a default-case/fallback reducer, as that would also be a building block for things like https://github.com/reduxjs/redux-toolkit/issues/259. In fact, I've been pondering that idea for a few weeks now. Also, this would have almost no impact on the typings. So nothing to fear there.

My point was that this issue did not read like @negebauer wanted a "fallback if no other reducer matched", but a "every action, even if another matches" reducer. And that was really irritating me.

negebauer commented 4 years ago

It is a “fallback if no other reducer matched” rather than a “every action, even if another marches”. No need to be irritating yourself.

phryneas commented 4 years ago

Okay, misunderstood you then :)

arnotes commented 4 years ago

ok my apologies. I just mean it would be nice to have an internal utility similar to this

type childSelector<P,C> = (state:P) => C;
export const forwardReducer = <P,C>(childSlice:Slice<C>,childSelector:childSelector<P,C>) => {
    const reducerDC = {} as any;//will fix types later
    for (const key in childSlice.actions) {
        if (childSlice.actions.hasOwnProperty(key)) {
            const action = childSlice.actions[key];
            reducerDC[action.type] = (state:P, action:any) => {
                childSlice.reducer(childSelector(state), action);
            }
        }
    }
    console.log({reducerDC, childSlice});
    return reducerDC;
}

and use it like this

export const teacherSlice = createSlice({
    name: 'teacher',
    initialState: {student:{}} as any as teacher,
    reducers:{
        setName:(state, action) => {
            state.teacherName = action.payload
        }
    },
    extraReducers: forwardReducer<teacher, student>(studentSlice, state => state.student)
})
phryneas commented 4 years ago

@arnotes Way to make yourself friends and get your opinion taken seriously.

Just in case you were not aware of that: there is no way of reading your comment as constructive feedback. Please watch your tone.

h0jeZvgoxFepBQ2C commented 4 years ago

Right now I have a usecase which I dunno how to solve, beside a default reducer?

On our servers we are just pushing underscore seperated action names, f.e.

EVENT_CREATED , EVENT_UPDATED , EVENT_DELETED

We don't want to change these names due some system constraints, so I'm not sure how to forward these messages, so that redux toolkit / the slices are recognizing them correctly (instead of using f.e. "event/created" directly)? (we are using cablecar btw: (https://github.com/ndhays/redux-cablecar)

phryneas commented 4 years ago

@h0jeZvgoxFepBQ2C createSlice's extraReducers can react to any action type, no matter how it is written. Also createReducers works with any action type.

And if those actions are not created with RTK, they shouldn't be in reducers anyways, as the idea of reducers is to create actions for these reducers as well.

h0jeZvgoxFepBQ2C commented 4 years ago

Yeah, I found this post which probably helps to solve my problems: https://github.com/reduxjs/redux-toolkit/issues/81#issuecomment-455881727 thanks!

phryneas commented 4 years ago

@h0jeZvgoxFepBQ2C No. That is still doing a createAction. From what I am reading, those actions already exist in your system, so you do not need any new action creators?

Just do

const reducer = createReducer(initialState, {
    EVENT_CREATED: () => {},
    EVENT_UPDATED: () => {},
    EVENT_DELETED: () => {},
});
h0jeZvgoxFepBQ2C commented 4 years ago

Probably, the actions are not really necessary anyway, since we don't call these actions in our app directly, but only get the cablecar dispatchs from the server.