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

Add ability for slices to listen to other actions #83

Merged
merged 9 commits into from Jan 25, 2019

Conversation

@markerikson
Copy link
Collaborator

markerikson commented Jan 21, 2019

Thus far, slices generated by createSlice could only listen to the specific action types defined for each field in the reducers option.

I've added a field named extraReducers that lets you add reducers that listen to action types that were defined elsewhere.

I'm open to bikeshedding suggestions on the field name. Other ideas:

otherReducers
listenForActions
handleActions

I also updated createAction so that action creators have the string available as a type field. This was requested in #72 for use with switch statements. Turns out it also helps with TS usage, because TS won't let you pass a function directly as a computed object property - it's not a string or any. So, [actionCreator.type] is a passable alternative.

The TS compiler seems to be okay with this at the moment, but this is the first time I've ever done anything meaningful with TS, so I'd appreciate review. (Paging @denisw , @Dudeonyx , and @Jessidhia ...)

I filled out the createSlice docs as well.

@netlify

This comment has been minimized.

Copy link

netlify bot commented Jan 21, 2019

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

Built with commit 4c85493

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

Copy link
Contributor

denisw left a comment

Looks legit to me from a TypeScript perspective, I left just two small comments (see below).

However, on a more general level, I would like to question the value createSlice() provides, and whether there should be such a helper in redux-starter-kit at all. The introduction in the library documentation says:

It does not address concepts like "reusable encapsulated Redux modules"

However, I feel like createSlice() definitely moves into that territory by bundling multiple Redux entities (reducer, action creators, one selector), and does so in a way that, to me, feels inferior to just writing a module that defines and exports these things separately.

Here are a few issues with the current createSlice() that don't really exist with the "just write a module" approach:

  • TypeScript issues. The approach of generating action creators implicitly from the reducers object keys and case reducer types seems to work for now, but is more fiddly and complex that it would be if the action creators were just defined and typed independently. More importantly, the dynamically named slice selector is impossible to type correctly (which, admittedly, could be fixed by giving the selector a fixed name).

  • No custom selectors. There is slice.selectors.get[SliceName](), which suggests that other selectors related to this slice should go there as well. This could certainly be made possible by adding a selectors option. However, this wouldn't really save any code, or add any convenience, compared to defining the selectors separately. Yet, again, it feels strange to have some selectors attached to slices and some not. It's weird.

  • No way to bundle thunk actions, sagas, epics, etc. This is of course out of scope for this library, but it does make the "slice" concept kind of strangely limited - I can bundle reducers, action creators and selectors belonging to the same slice of state, why can't I put my other related "non-core" things there as well? With a JavaScript module, this problem just doesn't occur - I can add whatever export I want.

I think that the extraReducers option you added is another sign of this awkwardness. It looks like purely an artifact of createSlice() doing a bit too much; using createReducer() directly makes this extra concept completely unnecessary.

To summarize, I feel that createSlice() lies too awkwardly in between "remove a little bit of boilerplate when defining a reducer plus some actions" and "approach to bundling related Redux code" that cannot appropriately serve all needs without effectively becoming a miniature version of the JS module system. I would rather recommend library users to write slices as modules, which doesn't take much more code and has none of the problems mentioned above.

/* counter.js */

// Selectors

export function getCounter(state) {
  return state.counter
}

// Actions

export const increment = createAction('increment')
export const decrement = createAction('decrement')

// Thunks, epics, sagas, ...

// Reducer

export default createReducer(0, {
  [increment]: (state, { payload }) => state + payload,
  [decrement]: (state { payload }) => state - payload
})
@@ -18,6 +18,7 @@ export interface PayloadAction<P = any, T extends string = string>
export interface PayloadActionCreator<P = any, T extends string = string> {
(): Action<T>
(payload: P): PayloadAction<P, T>
type: string

This comment has been minimized.

Copy link
@denisw

denisw Jan 21, 2019

Contributor

You can type type as T here. If the action type is statically known (like when you write createAction('foo'), then actionCreator.type will have the literal type 'foo' instead of the generic string.

@@ -15,6 +15,9 @@ import {
reducers: {
increment: (state: number, action) => state + action.payload,
decrement: (state: number, action) => state - action.payload
},
extraReducers: {
"OTHER_ACTION_TYPE" : (state : number, action ) => state + action.payload.count

This comment has been minimized.

Copy link
@denisw

denisw Jan 21, 2019

Contributor

This line looks as if it was not formatted with Prettier (unneeded quotes, space before the :). Does your editor run Prettier on TypeScript files?

This comment has been minimized.

Copy link
@markerikson

markerikson Jan 22, 2019

Author Collaborator

No, I don't have Prettier configured in my editor at all. Probably something I should set up.

@denisw denisw referenced this pull request Jan 21, 2019
8 of 12 tasks complete
@Dudeonyx

This comment has been minimized.

Copy link
Contributor

Dudeonyx commented Jan 22, 2019

@markerikson instead of creating a new field called extraReducers to enable the reducer listen to other actions it would be far easier to change this in createSlice.ts line: 102

  const actionMap = actionKeys.reduce(
    (map, action) => {
      const type = getType(slice, action)
      map[action] = createAction(type)
      return map
    },
    {} as any
  )

to

  const actionMap = actionKeys.reduce(
    (map, action) => {
      map[action] = createAction(action)  // <- this specifically
      return map
    },
    {} as any
  )

**Edit: The reducerMap in line: 95 becomes useless as well, since we can now directly forward the reducers to createReducer **

  const reducerMap = actionKeys.reduce((map, actionKey) => {
    map[getType(slice, actionKey)] = reducers[actionKey]
    return map
  }, extraReducers) 

By removing the getType() function which attaches the slice to the action type, the actionCreators type strings are no longer unique to each reducer, much like a regular switch-case reducer.

So instead of

const user = createSlice({
  slice: 'user',
  initialState: { name: '', age: 20 },
  reducers: {
    setUserName: (state, action) => {
      state.name = action.payload // mutate the state all you want with immer
    }
  },
  extraReducers: {
    [counter.actions.increment]: (state, action) => {
      state.age += 1
    }
  }
})

we do

const user = createSlice({
  slice: 'user',
  initialState: { name: '', age: 20 },
  reducers: {
    setUserName: (state, action) => {
      state.name = action.payload // mutate the state all you want with immer
    },
   [counter.actions.increment]: (state, action) => {
      state.age += 1
    }
  },
})
@markerikson

This comment has been minimized.

Copy link
Collaborator Author

markerikson commented Jan 22, 2019

@Dudeonyx : maybe I'm misreading the changes you're suggesting, but it seems like they break part of the point of what createSlice is intended to do.

I specifically want createSlice to generate prefixed action types if a slice name is provided. I also specifically want to distinguish between "these are action types that are 'owned' by this slice, generate action creators for them" and "these are action types from somewhere else, don't generate action creators".

@markerikson

This comment has been minimized.

Copy link
Collaborator Author

markerikson commented Jan 23, 2019

Closing this PR in favor of #86 , since that's a superset.

I just opened up #91 to discuss how createSlice ought to behave beyond this capability.

@markerikson markerikson reopened this Jan 25, 2019
Dudeonyx and others added 3 commits Jan 25, 2019
Implemented my proposed changes from #83 (comment)

@denisw  @markerikson your inputs are appreciated

*Note: this PR is for the `feature/other-slice-action` branch not `master`*

* ~~Removed `getType()` utility~~

* ~~`slice` is no longer attached to `actionsMap` and `reducerMap`~~

* ~~Removed `reducerMap` and directly forward `reducers` to `createReducer`~~

- [x] `createAction()` action creators `type` property returns a `string literal` for better type saftey

- [x] Fixed tests

- [x] Added tests
# Conflicts:
#	docs/api/createSlice.md
@markerikson

This comment has been minimized.

Copy link
Collaborator Author

markerikson commented Jan 25, 2019

Didn't realize #86 was really meant for this branch. Merged it over, and putting this in.

@markerikson markerikson merged commit affc5d7 into master Jan 25, 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
@markerikson markerikson deleted the feature/other-slice-actions branch Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.