“Flux Standard Action” has 3700+ stars on github and used by “redux-promise”, “redux-actions” and other libraries.
Several weeks ago one of my developers tried to switch to FSA approach in one of our projects. After some time of discussion it was decided that FSA is not what we need.
FSA has good ideas behind it. Main idea is to have a convention about Flux actions shape. It increases compatibility between different libraries and allows to create more consistent flux/redux ecosystem.
I like the idea of having “type” and “payload” fields. But there are some concerns related to error handling.
Everything is about conventions
One of the key ideas of the FSA is “errors as a first class concept”. FSA says that using LOAD_SUCCESS and LOAD_FAILURE is less than ideal. But in my opinion both approaches are just conventions. You can have convention like having an “error” property in an action
{
type: `${ACTION_NAME}`
error: `${STATUS}`
}or you can have another convention – action status as part of the action type
{
type: `${ACTION_NAME}_${STATUS}`
}I call it “tradional approach” in this post
In both cases you have errors as a first class concept (on a convention level). If you want more explicit errors then you can use something like ${ACTION_NAME}:${STATUS} instead of ${ACTION_NAME}_${STATUS}. So you will have a special syntax for separating action status.
Let’s look closer at the FSA appoach of error handling. It has substantial drawbacks in my opinion.
Double dispatch problem
First of all it is the double dispatch. We have the first dispatch based on the “type” field and the second dispatch based on the “error” field. An it is really painful.
For example, we can have actions that are not going to produce errors. For example, often you do not expect any concrete error for fetching operations (yes, we can have errors while fetching, but almost usually it’s general “Server error”). So, for fetch operations you can omit the error handling and write something like that:
function todos(state = [], action) {
switch (action.type) {
case 'LOAD_ALL_TODOS': {
return action.payload;
}
case 'RELOAD_TODO': {
return state.map(todo => action.payload.id === todo.id ? action.payload.id : todo);
}
case 'ADD_TODO' : {
return [...state, action.payload]
}
default: {
return state;
}
}
}But what if in future you will have an error in your ‘LOAD_ALL_TODOS’ action . In this case your action will go wrong way (for the success path) which can cause unpredictable bahavior.
So, you see that and decide that the best way is to always check the error flag for every action.
And you rewrite your code to:
function todos(state = [], action) {
switch (action.type) {
case 'LOAD_ALL_TODOS':
if (!action.error) {
return action.payload.todos;
}
break; // Do not forget about break
case 'RELOAD_TODO': {
if (!action.error) {
return state.map(todo => action.payload.id === todo.id ? action.payload.id : todo);
}
break;
}
case 'ADD_TODO' : {
if (!action.error) {
return [...state, action.payload];
}
break;
}
default: {
return state
};
}
}So, we should always have additional “if” statement. And now it looks like this annoying best practice about using “obj.hasOwnProperty(key)” in “for in” loops. That unconvenient. You will have this “if” statements everywhere for every action, even for button click. You are never going to have an error in this button click action but you must check for error for the consistency with the rest of the code. Otherwise, someday someone will forget this “if” check in another action (I believe that this will happen in any case).
Moreover, we’ve introduced a new bug occasionally. If we have an error in any of the supported action types, the reducer will return “undefined (as it will not match default statement). So, we change the code to:
function todos(state = [], action) {
switch (action.type) {
case 'LOAD_ALL_TODOS':
if (!action.error) {
return action.payload.todos;
}
break; // Do not forget about break
case 'RELOAD_TODO': {
if (!action.error) {
return state.map(todo => action.payload.id === todo.id ? action.payload.id : todo);
}
break;
}
case 'ADD_TODO' : {
if (!action.error) {
return [...state, action.payload];
}
break;
}
}
return state;
}But why do I need all of this. FSA actions are compared to promises but why I handle errors like in continuation style?
Compare above code to this one:
function todos(state = [], action) {
switch (action.type) {
case 'LOAD_ALL_TODOS:SUCCESS': {
return action.payload.todos;
}
case 'RELOAD_TODO:SUCCESS': {
return state.map(todo => action.payload.id === todo.id ? action.payload.id : todo);
}
case 'ADD_TODO:SUCCESS' : {
return [...state, action.payload];
}
default: {
return state;
}
}
}FSA action handling has higher cyclomatic complexity than the traditional approach.
Progress handling problem
The second issue with FSA errors approach. Is that we think that action can be only in two states (Why at all we are talking about state of the actions???). FSA is compared to a Promise in the docs. But the problem is that any promise has more than two states. As they represent async values, they can be in unfulfilled state. Promises represent result of the async execution. In FSA we think about actions as a result of execution but it’s always in the final state – success or failure, we do not have unfulfilled state.
In a traditional approach you can have ‘LOAD_ALL_TODOS:SUCCESS’, ‘LOAD_ALL_TODOS:FAILURE’, ‘LOAD_ALL_TODOS:INPROGRESS’ (or ‘LOAD_ALL_TODOS:REQUEST’ ).
function todos(state = [], action) {
switch (action.type) {
case 'LOAD_ALL_TODOS:SUCCESS': {
// Do something
}
case 'LOAD_ALL_TODOS:FAILURE': {
// Do something
}
case 'LOAD_ALL_TODOS:INPROGRESS' : {
// Do something
}
default: {
return state;
}
}
}I cannot do this with FSA. Of course, we can define another action like “LOAD_ALL_TODOS_INPROGRESS” but in this case:
- We switch back to the traditional approach
- It does not make any sense to have error flag for “LOAD_ALL_TODOS_INPROGRESS”.
- It contradicts with the idea of having one action type for representing loading of all todo items.
Handling all errors in global sequence of actions
With FSA it is easy to track all errors in you app.
function reducer(state = [], action) {
if ( action.error ) {
// Do something
}
}But you can do the same with the traditional approach:
function reducer(state = [], action) {
if ( action.type.endsWith(':FAILURE') ) {
// Do something
}
}Flux actions are not promises
I believe that it is incorrect to think that every action is a result (with error or not) of execution of some operation (that we run). For example, you can have actions coming from UI events like buttons clicks (it is unusal to get a failed button click). We can have streams of actions from a server (though websocket connection) containing notifications. Notifications can be about failure but actions itself should not have “error:true”. And don’t say, that eveything is the result of execution of some operation, it will be too general 🙂
“Flux actions can be thought of as an asychronous sequence of values”. Yes, absolutely. But we do not need additional level of asynchrony at the action level. We do not want to make every action represent asynchronous operation, we don’t want to make every action the result of execution of run operations. So, it is ok to have actions without addional statuses like :SUCCESS, :FAILURE or :INPROGRESS. We do not need :SUCCESS for button click.
One more thing that I do not like is that action shape depends not only on the action type but also on the flags inside the action (“error” flag) which is not a real problem but IMHO increases variability and complexity.
I like the ideas behind “FSA” but I believe that we should move forward towards “FSA v2” :).
