Skip to content

implement FSA errors by setting error to true if payload is an Error obj#16

Merged
acdlite merged 1 commit into
redux-utilities:masterfrom
ngokevin:fsaerrors
Nov 18, 2015
Merged

implement FSA errors by setting error to true if payload is an Error obj#16
acdlite merged 1 commit into
redux-utilities:masterfrom
ngokevin:fsaerrors

Conversation

@ngokevin
Copy link
Copy Markdown
Contributor

Noticed this spec of FSA wasn't yet implemented, and there's an issue for it.

This sets action.error to true if the payload is a direct instance of Error. This is the faintest touch of magic, but if it's documented, it makes sense.

Also fixes a possible error where we're trying to set action.meta even though action is a const.

r? @acdlite

@acdlite
Copy link
Copy Markdown
Contributor

acdlite commented Aug 24, 2015

Not sure about this... Feels like it's outside the scope of the module. if the payload is an error, that means someone caught the error and placed it there. Shouldn't it be up to them to set error to true?

@ngokevin
Copy link
Copy Markdown
Contributor Author

But there doesn't seem to be a way to set error to true with createAction. The only thing that can be set by the user is type and payload.

@acdlite
Copy link
Copy Markdown
Contributor

acdlite commented Aug 24, 2015

Oh duh I see what you mean. I was thinking about handleAction() for some reason — actually I dunno what I was thinking.

This seems reasonable then.

Comment thread src/createAction.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check won't work for subclasses of Error (e.g. TypeError)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks, updated to use instanceof.

@ngokevin
Copy link
Copy Markdown
Contributor Author

Updated to use instanceof

@ricardofbarros
Copy link
Copy Markdown

+1

@ngokevin ngokevin mentioned this pull request Sep 3, 2015
@krasnoperov
Copy link
Copy Markdown

+1

2 similar comments
@cappslock
Copy link
Copy Markdown

+1

@tkqubo
Copy link
Copy Markdown

tkqubo commented Sep 27, 2015

+1

@jonny-improbable
Copy link
Copy Markdown

+1, please can you consider releasing this, @acdlite?

@bravewick
Copy link
Copy Markdown

+1

@pbomb
Copy link
Copy Markdown

pbomb commented Oct 7, 2015

I'm running into this issue now as well. Is this issue expected to be resolved soon?

@tokenvolt
Copy link
Copy Markdown

+1

@danilotorrisi
Copy link
Copy Markdown

I've also had the same issue, I hope it's gonna be released soon!

Comment thread src/createAction.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for changing this from const to let?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess i didn't have to. thought maybe i should since we modify action below, but we don't reassign.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it's less about the semantics of the keyword and more about consistency with the rest of the code base. const is used whenever a variable does not need to be reassigned.

@lukewestby
Copy link
Copy Markdown
Contributor

@ngokevin this is awesome, thanks for submitting! Can you update the README to document your change?

Can anyone think of a scenario where this would be a breaking change? I can't think of anything but if anyone can then we will need @acdlite's input before merging.

@ngokevin
Copy link
Copy Markdown
Contributor Author

@lukewestby thanks for checking it out, updated.

@Strate
Copy link
Copy Markdown

Strate commented Nov 3, 2015

👍

@kpaxqin
Copy link
Copy Markdown

kpaxqin commented Nov 17, 2015

@ngokevin
seems we can't customize the error object by payloadCreator.
maybe we can do in this way

export default function createAction(type, actionCreator, metaCreator) {
  const finalActionCreator = typeof actionCreator === 'function'
    ? actionCreator
    : identity;

  return (...args) => {
    const payload = finalActionCreator(...args)
    const action = {
      type,
      payload,
      error: (payload instanceof Error)
    };

    if (typeof metaCreator === 'function') {
      action.meta = metaCreator(...args);
    }

    return action;
  };
}

acdlite added a commit that referenced this pull request Nov 18, 2015
implement FSA errors by setting error to true if payload is an Error obj
@acdlite acdlite merged commit 9eca288 into redux-utilities:master Nov 18, 2015
@acdlite
Copy link
Copy Markdown
Contributor

acdlite commented Nov 18, 2015

Huzzah! I finally got off my ass and merged this! Thanks!

@kpaxqin
Copy link
Copy Markdown

kpaxqin commented Nov 19, 2015

@acdlite
I've create a PR to set error to true when payloadCreator return Error object.
see: #43

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.