Skip to content

Remove FSA check in handleAction(s) #168

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

Merged
merged 1 commit into from
Nov 21, 2016
Merged

Remove FSA check in handleAction(s) #168

merged 1 commit into from
Nov 21, 2016

Conversation

timche
Copy link
Member

@timche timche commented Nov 21, 2016

With #141 we have made other libraries incompatible with redux-actions and it resulted to an unexpected behavior (#164, #167). If an action is not a FSA, handleAction should not handle it and just pass it to the next reducer. This fix will remove the FSA check to support Non-FSA.

Closes #164, #167

@timche timche force-pushed the fix/handle-non-fsa branch 2 times, most recently from 12d5542 to 9da590b Compare November 21, 2016 19:17
Copy link
Contributor

@yangmillstheory yangmillstheory left a comment

Choose a reason for hiding this comment

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

Does there need to be a change in the README as well?

'The FSA spec mandates an action object with a type. Try using the createAction(s) method.'
);

if (!includes(actionTypes, action.type.toString())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

const {type: actionType} = action

or

const {type} = action

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it.

@timche
Copy link
Member Author

timche commented Nov 21, 2016

We didn't made any changes when we added the check. The current README makes it pretty clear that handleAction only handles FSA.

}
})).to.deep.equal({
counter: 0
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a chai assertion that we expect this code block not to throw? I think that's the point, and not that the state is reduced to a certain value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This reducer should not throw AND just return the state. Will change that.

With #141 we have made other libraries incompatible with redux-actions and it resulted to an unexpected behavior (#164, #167). If an action is not a FSA, handleAction should not handle it and just pass it to the next reducer. This fix will remove the FSA check to support Non-FSA.

Closes #164, #167
@timche timche force-pushed the fix/handle-non-fsa branch from afb556f to ef4acb7 Compare November 21, 2016 19:29
@timche
Copy link
Member Author

timche commented Nov 21, 2016

Please review again @yangmillstheory.

@yangmillstheory
Copy link
Contributor

Just have one last question: #168 (comment)

@timche
Copy link
Member Author

timche commented Nov 21, 2016

Already answered and added :P

@yangmillstheory
Copy link
Contributor

Was this test failing before the application code changed?

@timche timche merged commit ef4acb7 into master Nov 21, 2016
@timche
Copy link
Member Author

timche commented Nov 21, 2016

What do you mean exactly?

@timche timche deleted the fix/handle-non-fsa branch November 21, 2016 20:02
@yangmillstheory
Copy link
Contributor

If we removed the FSA check removal, then the test should fail (TDD) to eliminate false positives. Just wondering if that's the case

@timche
Copy link
Member Author

timche commented Nov 21, 2016

The test I've wrote was failing when I've just removed the checks, because type was missing.

@yangmillstheory
Copy link
Contributor

Great, thanks :)

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.

Non-FSA actions cause handleAction reudcers to emit confusing error messages
2 participants