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

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

Merged
merged 1 commit into from
Nov 18, 2015

Conversation

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

type,
payload: finalActionCreator(...args)
};

if (typeof metaCreator === 'function') action.meta = metaCreator(...args);
if (args.length === 1 && args[0].constructor === Error) {
Copy link
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
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
Contributor Author

Updated to use instanceof

@ricardofbarros
Copy link

+1

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

+1

2 similar comments
@cappslock
Copy link

+1

@tkqubo
Copy link

tkqubo commented Sep 27, 2015

+1

@jonny-improbable
Copy link

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

@bravewick
Copy link

+1

@pbomb
Copy link

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

+1

@danilotorrisi
Copy link

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

@@ -8,12 +8,19 @@ export default function createAction(type, actionCreator, metaCreator) {
: identity;

return (...args) => {
const action = {
let action = {
Copy link
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
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
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
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
Contributor Author

@lukewestby thanks for checking it out, updated.

@Strate
Copy link

Strate commented Nov 3, 2015

👍

@kpaxqin
Copy link

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
Contributor

acdlite commented Nov 18, 2015

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

@kpaxqin
Copy link

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.