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

What is the reason error property is boolean? #17

Open
gajus opened this issue Sep 2, 2015 · 19 comments
Open

What is the reason error property is boolean? #17

gajus opened this issue Sep 2, 2015 · 19 comments

Comments

@gajus
Copy link

gajus commented Sep 2, 2015

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
Copy link

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
Copy link
Author

gajus commented Sep 3, 2015

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
Copy link

tomatau commented Sep 30, 2015

+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
Copy link

pbomb commented Oct 11, 2015

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
Copy link

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
Copy link
Author

gajus commented Oct 15, 2015

Which is a good thing.

On Oct 15, 2015, at 10:42, Malte Legenhausen [email protected] 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
Copy link

tomatau commented Oct 15, 2015

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
Copy link

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
Copy link

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
Copy link

tomatau commented Oct 15, 2015

@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
Copy link

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
Copy link

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
Copy link

👍 I concur also. Ditch the boolean on the error attribute and store the error there.

@thedumbtechguy
Copy link

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
Copy link

jchook commented Apr 28, 2016

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
Copy link

tomatau commented Apr 28, 2016

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
Copy link

sagiavinash commented Jun 5, 2016

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

@migueloller
Copy link

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
Copy link

@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

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

No branches or pull requests