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

RFC: Add predicate to createSlice reducers. #366

Closed

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Feb 16, 2020

This would make it more natural to write slice reducers as state machines.

It's just an idea on how to handle this and we'll definitely release 1.3 before we'll add this, but why not get some early feedback? :)

@davidkpiano, I think especially your feedback would be very valuable. Could you take a look?

Usage would look like this:

const slice = createSlice({
name: 'test',
initialState: {
status: 'idle'
} as { status: 'working' | 'idle'; workItem?: string },
reducers: {
startWork: {
predicate: state => state.status === 'idle',
reducer(state, action: PayloadAction<string>) {
state.status = 'working'
state.workItem = action.payload
}
},
finishWork: {
predicate: state => state.status === 'working',
reducer(state) {
state.status = 'idle'
state.workItem = undefined
}
}
}
})

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@city41
Copy link

city41 commented Feb 16, 2020

I'm just a random redux user who saw your tweet. I have to admit I don't think I'm a fan of this. I like that reducers currently are just functions. Making them objects with specific keys increases the cognitive load and required learning a tad, but with not much benefit because the predicate can just be an if statement at the top of the reducer function. I also think I would prefer to keep everything about the reducer contained in the function.

On the other hand, it does make the idea that reducers should be state machines a bit more explicit.

@davidkpiano
Copy link

While this does help, predicates don't necessarily make it explicit that the behavior is for a specific state (multiple predicates may return true, as they are not mutually exclusive).

A helper for state machines would have to deviate from createSlice, which is action-first by its nature. Here's an idea:

const userStatesReducer = createStates({
  key: 'status', // the key where the finite state lives
  initial: {
    status: 'idle',
    user: null,
    error: null
  },
  reducers: {
    // determined based on the key location
    idle: (state, action) => {/* ... */},
    loading: (state, action) => {/* ... */},
    failure: (state, action) => {/* ... */},
    success: (state, action) => {/* ... */}
  }
});

IMO, that API feels pretty nice! What do you think?

@davidkpiano
Copy link

Actually, you can combine the above idea with createSlice and have a really nice, state-machine-based interface:

const userMachine = createStates({
  key: 'status', // the key where the finite state lives
  initialState: {
    status: 'idle',
    user: null,
    error: null
  },
  reducers: {
    idle: {
      // same as `.reducers` in createSlice()
      fetch: (state, action) => {/* ... */}
    },
    loading: {
      cancel: (state, action) => {/* ... */},
      resolve: (state, action) => {/* ... */},
      reject: (state, action) => {/* ... */}
    },
    failure: {
      fetch: (state, action) => {/* ... */}
    },
    success: {
      fetch: (state, action) => {/* ... */}
    },
  }
});

dispatch(userMachine.actions.fetch(42)); // same as createSlice()

@phryneas
Copy link
Member Author

I'm just a random redux user who saw your tweet. I have to admit I don't think I'm a fan of this. I like that reducers currently are just functions. Making them objects with specific keys increases the cognitive load and required learning a tad, but with not much benefit because the predicate can just be an if statement at the top of the reducer function. I also think I would prefer to keep everything about the reducer contained in the function.

On the other hand, it does make the idea that reducers should be state machines a bit more explicit.

This is not a new notation - we are already using this notation for an optional prepare function, so this would just extend that existing notation. Of course, the "simple" syntax of passing just a normal reducer function will definitely be possible in the future, no matter how we decide on this.

@phryneas
Copy link
Member Author

phryneas commented Feb 16, 2020

@davidkpiano

(multiple predicates may return true, as they are not mutually exclusive).

I'm not sure if I really brought across what these would do (that alone is valuable feedback).

Multiple predicates being true would not be a problem, as these reducers are still bound to their respective action - the startWork predicate and reducer would only be evaluated if a test/startWork action would be dispatched.

Actually, you can combine the above idea with createSlice and have a really nice, state-machine-based interface

As the notation from above just extends an existing notation that takes an object with the keys reducer and prepare, we can't create a completely different functionality with overlapping semantics.

So, I'll take a more detailed step of explaining what this should do.

Before this RFC, you would currently be writing this:

const slice = createSlice({
    name: 'test',
    initialState: {
        status: 'idle'
    } as { status: 'working' | 'idle'; workItem?: string },
    reducers: {
        startWork(state, action: PayloadAction<string>) {
            if (state.status !== 'idle') {
                return;
            }
            state.status = 'working'
            state.workItem = action.payload
        },
        finishWork(state) {
            if (state.status !== 'working') {
                return
            }
            state.status = 'idle'
            state.workItem = undefined
        }
    }
}) 

This new predicate parameter would just move that early exit condition out of the reducer.

In the end, it would be possible to define something like

function fromStatus(status) {
  return (state) => state.status === status;
}

That could either (old notation) be used as

        finishWork(state) {
            if (!fromStatus(state, 'working')) { // I tweaked it here for this example to keep it legible
                return
            }
            state.status = 'idle'
            state.workItem = undefined
        }

or with the new notation like

        finishWork: {
            predicate: fromStatus('working'),
            reducer(state){
                state.status = 'idle'
                state.workItem = undefined
            }
        }

The point here is essentially not to create a completely new functionality (for that, we could go WAY better than what I suggested), but to enhance the existing functionality of createSlice to make it easier to work with state machines.
That said, of course we might still be adding something like createStates in the future, but that doesn't feel like a short-term goal.

PS: maybe predicate is not the right word? Non-english-native-speaker here. Would condition or precondition make more sense?

@markerikson
Copy link
Collaborator

I have mixed thoughts on this.

  • In one sense, this doesn't provide any new capabilities. As the examples show, you can already manually check states and make logic exclusive in a case reducer.
  • But, having a bit of API syntax for this specific case may make it more apparent and cut down the repetition a bit.
  • As @phryneas said, we've already got createSlice set up to take just a case reducer function as the default syntax, or {prepare, reducer} if you need to customize the generated action creator. So, the object field syntax exists, we'd just be adding a new possible key in there.
  • Switching from a function to an object of functions doesn't really gain anything in terms of saving the couple lines of code needed for if(someCondition) { ... }
  • I'm on the fence as to whether it actually really adds any real benefit, or if it would be better to leave createSlice alone and either add a separate function that's more focused on state machines, or just recommend that folks grab @xstate/fsm and be done with it.

@phryneas
Copy link
Member Author

It was a nice idea, but this is probably really not necessary. Closing :)

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