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

Changed FSA type dispatched on network errors to failureType #26

Closed
wants to merge 3 commits into from
Closed

Conversation

sgoll
Copy link

@sgoll sgoll commented Dec 1, 2015

Please see issue #25 for the rationale behind this pull request.

Basically, we want to make sure that the following invariant always holds true: for each [CALL_API] action that is handled by redux-api-middleware,

  • exactly one request FSA is dispatched, and
  • that request FSA is followed by either:
    • nothing (when the request FSA had its error property set), or
    • either a single success FSA or a single failure FSA (when the request FSA had its error property unset).

The actual code change is literally a single line, but the accompanying documentation change is rather more elaborate and might require some review.

Also, since the third argument res might now be either present or absent from the failure type descriptor's custom payload and meta properties (when using functions), this change is not backward compatible.

… the following invariant always holds true: at most one requestType FSA is dispatched, followed by either a single successType or a single failureType FSA, or nothing when the requestType FSA had its error property set.
@barrystaes
Copy link
Contributor

I agree with your point.
There should be only 1 request and errors should be in a failure FSA.

About the implementation;

  • I dont really understand the "res" sidenote you end with.
  • Should we do the same for the FSA resulting from a bailout which also used the requestType?
  • Are there more places requestType is used to return errors?
  • There is a merge conflict in this PR. Could you rebase it to the latest version?

And since this touches functionality i'd like @agraboso to give a green light for merging.

@sgoll
Copy link
Author

sgoll commented Feb 11, 2016

I updated the pull request.

@barrystaes:

Previously the following invariant was true: when the failure type descriptor defines custom payload and meta functions, those are called with three arguments: (action, state, res). With the changes done in this PR, this is no longer true: res might not be available when issuing a failure FSA, i.e. those functions might receive res === undefined. This may or may not be a problem.

Regarding your first question: I think it is okay to bail out with an error'd request FSA. The motivation for this issue was only that there could be multiple request FSAs. However, when the (only) request FSA received is marked as error that would be okay with me. It's only the two request FSAs (first without error, second with error) that I have a problem with and which should be fixed by this PR.

@barrystaes
Copy link
Contributor

Better late than never: thank you for the PR update.
Ok i now understand this is not about the request FSA with error flag.

Since this is a breaking change, this PR should go to the next v2.0.0-beta branch (instead of v1.1.x). @sgoll what do you think? Also, i wonder i you/anyone uses these changes in production?

@johnwiseheart
Copy link

Any update on this?

thessem added a commit to Proximaio/redux-api-middleware that referenced this pull request Jun 15, 2016
nason added a commit to nason/redux-api-middleware that referenced this pull request Mar 3, 2018
Previously:

RSAA -> Request FSA -> Request Error FSA

Now:

RSAA -> Request FSA -> Failure FSA

Closes agraboso#26 agraboso#44 agraboso#99
@nason nason modified the milestones: 2.0.0, 3.0 Mar 3, 2018
@nason
Copy link
Collaborator

nason commented Mar 3, 2018

This will be changed in #175

@nason nason closed this Mar 3, 2018
nason added a commit that referenced this pull request Mar 3, 2018
Previously:

RSAA -> Request FSA -> Request Error FSA

Now:

RSAA -> Request FSA -> Failure FSA

Closes #26 #44 #99
nason added a commit that referenced this pull request Jun 10, 2018
Previously:

RSAA -> Request FSA -> Request Error FSA

Now:

RSAA -> Request FSA -> Failure FSA

Closes #26 #44 #99
nason added a commit that referenced this pull request Jun 10, 2018
Previously:

RSAA -> Request FSA -> Request Error FSA

Now:

RSAA -> Request FSA -> Failure FSA

Closes #26 #44 #99
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants