redux-utilities / flux-standard-action

A human-friendly standard for Flux action objects.
MIT License
4.75k stars 142 forks source link

What is the reason error property is boolean? #17

Open gajus opened 9 years ago

gajus commented 9 years ago

as opposed to an instance of Error object. Boolean flag is vague. It just says what the error is and does not standardise the method of getting error description. Docs say that payload by convention should be an Error object.

cesarandreu commented 9 years ago

I think the reasoning might be so that you can serialize the action (which requires converting the Error object into a plain object) but still be able to convert the serialized action into its original state.

gajus commented 9 years ago

Ah, OK. It is not a problem then. I was working on the Canonical Composition Action spec. If you do not require that object is an instance of Error and instead specify certain traits (e.g. it must have message property), then object can be serialised and unserialised as needed.

By the way, I am happy if FSA copy from CCA everything. I am only trying to improve the standard. I have little hope for replacing the standard.

tomatau commented 9 years ago

+1 for error property being any object with a message property.

You can serialise an Error object:

JSON.stringify(new Error("my message"), ['message'])
// "{"message":"my message"}"

You can even serialise the stack if you so wish...

Having an error boolean flag to me seems like a duplication of action information. It's good to have a place for a standardised error format that can hold general information. Storing state specific IDs for failure in the meta data also seems like the wrong place.

For example, an action sequence that performs a batch update with optimism.

{ type: BATCH_ITEM_UPDATE_PENDING, payload: {items[]}, meta: {actionId} }
{ type: BATCH_ITEM_UPDATE_SUCCESS, payload: {successful_item_ids[]}, meta: {actionId} }
{ type: BATCH_ITEM_UPDATE_FAIL, payload: {failed_items[]}, meta: {actionId} }

The meta property contains an id specific to the action sequence -- not the state changes. The payload contains multiple ids (and more), for any items that failed and needs reverting from the optimistic update (or whatever else -- maybe error messages in the app?).

pbomb commented 9 years ago

I'm struggling a bit with this design as well. Our application has the typical pattern of fetches dispatching REQUEST, SUCCESS and FAILURE actions. For all of these actions, the payload always contains the contextual information required to identify which part of the state tree its for. If the FAILURE action is supposed to have the error property set to true and the payload property set to the error object, then the contextual properties don't have a home.

This is unless contextual information is always supposed to go into the meta field. My questions is related to the question in issue #18 asking what is supposed to go into the meta field. If contextual info should go there, then the REQUEST action would not have a payload, but only have meta. Can someone explain how this typical fetch pattern works with Flux Standard Actions?

mlegenhausen commented 9 years ago

Forcing the assignment of error to the payload property seems to force you to use custom error objects, that contain all necessary information to describe the error context.

gajus commented 9 years ago

Which is a good thing.

On Oct 15, 2015, at 10:42, Malte Legenhausen notifications@github.com wrote:

Forcing the assignment of error to the payload property seems to force you to use custom error objects, that contain all necessary information to describe the error context.

— Reply to this email directly or view it on GitHub.

tomatau commented 9 years ago

I agree that It's good to enforce standards and common structures but I don't think the payload is the best place for that when handling an error action.

Right now we already force the error field to be a boolean, which is clearly limited in the information it can communicate -- so seems like a better place to enforce custom error objects. Then the payload doesn't need to conform to a structure when in an error action and can be free to contain more data about the error-action.

bryanlarsen commented 9 years ago

The error design of Flux Standard Actions is completely unworkable for us; we've decided to not use Flux Standard Actions for that reason.

  1. As mentioned, the 'error' boolean is redundant. For us, an error is any action that has a type that ends with 'ERROR'.
  2. We almost always require the original payload when handling error actions. For example, when we have an error updating a model, we update our state to contain the list of field that have not been successfully updated. It's certainly possible to put the original payload into meta.payload or payload.payload or something like that, but that's just silly. In our "FSA", the payload property of an error action is always the original payload.
  3. Given that most error objects are the result of HTTP actions, we have a standard field that contains the HTTP status code. Not all errors have to have this field, but when they do, it's in a standard place.
  4. We make the Error object required rather than optional, and put it in the error property of the action.
mlegenhausen commented 9 years ago

Using an error flag is more explicit in term of what is possible with javascript. If you want to use a falsy value as your "error object"/payload you are unable to determine if your action is an error or not.

Example:

// works
{
   error: {
      status: 404
   }
}

{
  error: 'Error message'
}

// does not work
{
  error: ''
}

// works again
{
  error: true
  payload: { status: 404 }
}

{
   error: true
   payload: ''
}
tomatau commented 9 years ago

@mlegenhausen -- you can have a falsy value in your payload if you conform to the error value should always have a message property -- like an Error object, then everything else that doesn't have a message property can be ignored.

No-one here is suggesting that passing an empty string (or any other ambiguous falsy value) should be used as an error value.

mattkrick commented 8 years ago

I agree with where this is going. If there's an error, it's in my action type & that action creator can handle the error. What I don't like is having the error itself in the payload instead of the action that caused the error (or the multiple failed items if it's a batch job). I say let error be an Error object & for those edge cases where the action type doesn't have an ERROR/FAIL suffix, they can action.error instanceof Error (after a rehydrate, if serialized).

m-sanders commented 8 years ago

I agree with the general sentiment of this issue.

I personally am using something akin to Error Handling in Nodejs guidelines, where only ever one property of data or error is set, error is always an instance of Error with an appropriate computer friendly name and a human friendly message. Any additional context objects can be attached to Error and should consistently exist for all instances of Error with the same name. On success data is a free for all but I try to document all relevant data on the action. I am using data over payload - both are equally vague but data is less keystrokes!

Anyway, I am sure there is an XKCD mentioning something about competing standards....

MarkMurphy commented 8 years ago

:+1: I concur also. Ditch the boolean on the error attribute and store the error there.

thedumbtechguy commented 8 years ago

I came from SO for this same issue.

When an error occurs, it IS an action. It is not an action with an error.

I forsee conditional logic sprinkled all over your action handling code checking for an error property when you could simply be handling an error action.

If you are using something like redux, you can have a dedicated reducer handling errors.

A better design would have been to standardize the error payload so handling is streamlined.

Another thing is properties that 'MAY' exist. How is that even standard then? More conditional logic for me.

An action should have ONLY two properties. A TYPE and a PAYLOAD. Everything else, error messages, meta data etc should be in the PAYLOAD. That is a standard.

jchook commented 8 years ago

Consider this ideal use case where an AJAX request has 3 prongs:

  1. initial dispatch({ type, payload })
  2. success dispatch({ type, payload, meta: { serverResponse }})
  3. error dispatch({ type, payload, error, meta: { serverResponse }})

Wouldn't it make the most sense to have the payload consistent across all three in order to tell what parameters led to error / success? Unless I keep the original action in the action-creator scope, then put it in meta (yo dawg I heard you like actions), I cannot see what the payload of the initial action was.

tomatau commented 8 years ago

The parameters causing an error can be put into an Error object -- along with many other things.

I would assume it's more common for server responses to be placed in payload, not the meta object -- which makes the payload inconsistent already.

sagiavinash commented 8 years ago

we can have a hasError flag in meta object and payload be the error object.

migueloller commented 8 years ago

I think the original intent of adding error as a boolean property can be deduced from the example in the readme.

{
  type: 'ADD_TODO',
  payload: {
    text: 'Do something.'  
  }
}

{
  type: 'ADD_TODO',
  payload: new Error(),
  error: true
}

There are no redundancies in this example.

Due to the way people use Redux, though, it starts to get redundant.

{
  type: 'ADD_TODO_REQUEST',
  payload: {
    text: 'Do something.'  
  }
}

{
  type: 'ADD_TODO_SUCCESS',
  payload: {
    text: 'Do something.'
  }
}

{
  type: 'ADD_TODO_FAILURE',
  payload: new Error(),
  error: true
}

Perhaps the correct approach would be to separate app concerns from asynchronous concerns and instead of doing request -> success | failure, it's better to do request -> success (triggers action<foo>) | failure (triggers error<foo>), where action<foo> is equivalent to { type: 'foo' } and error<foo> is equivalent to { type: 'foo', error: true } with their respective payloads.

The only issue this brings are optimistic updates. This could be solved, though, by triggering actions that queue the change when the request is made and then committing when it succeeds or reverting if it fails.

thedumbtechguy commented 8 years ago

@migueloller

Exactly my point. What you should be doing is standardizing the payload for a given action your application. That way, your app knows how to react when ever it sees that action type.

What happens when you decide that for a certain error, you want to do something different for example?

You