Dispatch failure FSA when fetch fails #175

nason · 2018-03-03T17:48:05Z

Previously:

RSAA -> Request FSA -> Request Error FSA

Now:

RSAA -> Request FSA -> Failure FSA

Closes #26 #44 #99

coveralls · 2018-03-03T17:56:34Z

Coverage remained the same at 100.0% when pulling fda1dc5 on failed-req-failure-fsa into af94473 on master.

coveralls · 2018-03-03T17:56:34Z

Coverage remained the same at 100.0% when pulling 96b0691 on failed-req-failure-fsa into 836a3d3 on next.

coveralls · 2018-03-03T17:56:34Z

Coverage remained the same at 100.0% when pulling fda1dc5 on failed-req-failure-fsa into af94473 on master.

darthrellimnad · 2018-04-28T03:20:23Z

First off, I love this lib! 🎉

Second, I personally welcome this change :). Checking for the .error flag on the REQUEST action was always kinda a pain and source of confusion for new developers.

However, this is a pretty big breaking change, since now FAILURE actions can return the the RequestError type, whereas (i think) this wasn't possible before. Depending on the project, this could be significant?

do you think there would be a way to support a "classic" mode of sorts? I wouldn't want it included if it added a lot of overhead, but might help with migration from 2 -> 3 among community :)

darthrellimnad · 2018-04-28T03:39:04Z

src/middleware.js

@@ -160,7 +160,7 @@ function apiMiddleware({ getState }) {
        return next(
          await actionWith(
            {
-              ...requestType,
+              ...failureType,


maybe a "migration" helper option could exist? like:

...(options.legacyRequestError ? requestType : failureType),

maybe until v4 or something, although not sure if anyone else might find this useful, can always stick with v2 till you're ready for migration too :)

I'm open to a migration helper, but I don't know what the right approach is here. A breaking change that:

requires you to go in and add a legacyRequestError: true to your RSAAs as a temporary fix

OR

requires you to update your error handling paths to this new spec

I lean towards the latter but can be convinced otherwise!

craig0990 · 2018-05-02T07:01:55Z

OK, I've just bumped into this on my first run-through with the library (because my API isn't accessible right this second - I figured I'd boot it up when I got the code working).

Obviously, things are fine as they are right now (now that I'm checking for .error in REQUEST-type actions), but it was confusing to begin with 😀

Just adding my tuppence, and eagerly awaiting 3.0 🙃

Edit: just reading through the Lifecyle in the README, and while in a lot of cases it would be sufficient to dispatch a FAILURE FSA, perhaps there's a case for an ERROR FSA, to distinguish between where the API was actually contacted, but failed, and where the API couldn't be called at all.

I can see us potentially using re-dispatching an ERROR FSA so we could gracefully handle offline functionality, for example.

darthrellimnad · 2018-05-03T02:14:41Z

Agree :) although as things stand, the Request FSA w/ error flag should indicate the "failed to contact API" case? So with the change from the PR, seems like we would need to test the payload for the error types (RequestError vs ApiError... or InternalError I guess, but those can be avoided).

What would a test look like for the new Failure FSA case for RequestError? Would we do a payload instanceof RequestError?

EDIT: looks like we could also use the name property of the error payload of the "FAILURE' FSA to switch around the error type as well:
https://github.com/agraboso/redux-api-middleware#requesterror

nason

I've been a bit distracted but want to pick this back up and get this change out!

Thanks for the great feedback @craig0990 + @darthrellimnad. Can you share any idea or samples of how you think this PR could be extended to give more information about what type of error happened?

nason · 2018-06-08T19:12:32Z

src/middleware.js

@@ -160,7 +160,7 @@ function apiMiddleware({ getState }) {
        return next(
          await actionWith(
            {
-              ...requestType,
+              ...failureType,


I'm open to a migration helper, but I don't know what the right approach is here. A breaking change that:

requires you to go in and add a legacyRequestError: true to your RSAAs as a temporary fix

OR

requires you to update your error handling paths to this new spec

I lean towards the latter but can be convinced otherwise!

Previously: RSAA -> Request FSA -> Request Error FSA Now: RSAA -> Request FSA -> Failure FSA Closes #26 #44 #99

nason · 2018-06-10T17:25:07Z

Merging this into the next branch for a 3.0 release. Open to tweaking this braking change further and adding different migration paths -- feel free to comment here or open PRs!

craig0990 · 2018-06-11T04:09:49Z

I can't quite remember exactly what I was thinking when I wrote the earlier comment :P I think it was around the fact that my API code had failed, but I had naively expected the FAILURE case to be called, and it wasn't.

A loading key as part of my reducers is one of my main use-cases. With the current implementation, I have to catch errors in two places.

Here's an example of me handling the REQUEST action to account for that:

      REQUEST: (state, action) => {
        return {
          ...state,
          meta: action.meta,
          // Requests may error in certain situations
          error: action.error,
          // We need to stop loading once we detect an error
          loading: !action.error,
          initialised: true
        }

By having the FAILURE FSA dispatched, I'd be able to simplify my code by explicitly setting loading to true on REQUEST, and false on FAILURE, regardless of whether the error was actually caused by mis-configuration or a bad response.

Hope that clarifies :)

nason · 2018-06-11T14:06:17Z

@craig0990 thanks. I hope this change makes exactly that possible! It's currently in the next branch, from which I'm hoping to cut a 3.0 beta very soon.

I'll leave an update here when that happens 🍎

nason · 2018-06-14T03:05:47Z

This has been released in 3.0.0-beta.0

This is a follow-up to #175, which updated this behavior and docs for failed fetches, to include failures in RSAA custom methods, such as `[RSAA].headers`

nason mentioned this pull request Mar 3, 2018

Changed FSA type dispatched on network errors to failureType #26

Closed

nason added this to the 3.0.0 milestone Mar 3, 2018

nason force-pushed the failed-req-failure-fsa branch from e68c94f to fda1dc5 Compare March 3, 2018 17:51

nason added enhancement Breaking Change labels Mar 3, 2018

nason mentioned this pull request Mar 3, 2018

On next, dispatches requestType rather than failureType for fetch errors #99

Closed

nason mentioned this pull request Mar 22, 2018

Handling RequestErrors status codes #178

Open

darthrellimnad reviewed Apr 28, 2018

View reviewed changes

nason commented Jun 8, 2018

View reviewed changes

nason changed the base branch from master to next June 10, 2018 16:36

nason force-pushed the failed-req-failure-fsa branch from fda1dc5 to ebf15ec Compare June 10, 2018 16:39

nason added 2 commits June 10, 2018 12:45

Dispatch failure FSA when fetch fails

9fbbc38

Previously: RSAA -> Request FSA -> Request Error FSA Now: RSAA -> Request FSA -> Failure FSA Closes #26 #44 #99

Update upgrading from 2.x section of readme

96b0691

nason force-pushed the failed-req-failure-fsa branch from ebf15ec to 96b0691 Compare June 10, 2018 16:46

nason merged commit a0460d2 into next Jun 10, 2018

nason deleted the failed-req-failure-fsa branch June 17, 2018 05:31

This was referenced Jul 16, 2018

Migrate tests to jest #203

Merged

Revisit error handling w/ 3.0 RSAA spec change #205

Closed

nason added a commit that referenced this pull request Jul 16, 2018

Update handling of failure in RSAA descriptor custom methods

f0ca6fb

This is a follow-up to #175, which updated this behavior and docs for failed fetches, to include failures in RSAA custom methods, such as `[RSAA].headers`

nason mentioned this pull request Jul 16, 2018

Update handling of failure in RSAA descriptor custom methods #206

Merged

iamandrewluca mentioned this pull request Feb 24, 2020

What is the reasoning for a Request FSA with error flag? (vs a Failure FSA) #44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dispatch failure FSA when fetch fails #175

Dispatch failure FSA when fetch fails #175

nason commented Mar 3, 2018

coveralls commented Mar 3, 2018

coveralls commented Mar 3, 2018 •

edited

Loading

coveralls commented Mar 3, 2018

darthrellimnad commented Apr 28, 2018

darthrellimnad Apr 28, 2018 •

edited

Loading

nason Jun 8, 2018

craig0990 commented May 2, 2018 •

edited

Loading

darthrellimnad commented May 3, 2018 •

edited

Loading

nason left a comment

nason Jun 8, 2018

nason commented Jun 10, 2018

craig0990 commented Jun 11, 2018

nason commented Jun 11, 2018

nason commented Jun 14, 2018

Dispatch failure FSA when fetch fails #175

Dispatch failure FSA when fetch fails #175

Conversation

nason commented Mar 3, 2018

coveralls commented Mar 3, 2018

coveralls commented Mar 3, 2018 • edited Loading

coveralls commented Mar 3, 2018

darthrellimnad commented Apr 28, 2018

darthrellimnad Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

nason Jun 8, 2018

Choose a reason for hiding this comment

craig0990 commented May 2, 2018 • edited Loading

darthrellimnad commented May 3, 2018 • edited Loading

nason left a comment

Choose a reason for hiding this comment

nason Jun 8, 2018

Choose a reason for hiding this comment

nason commented Jun 10, 2018

craig0990 commented Jun 11, 2018

nason commented Jun 11, 2018

nason commented Jun 14, 2018

coveralls commented Mar 3, 2018 •

edited

Loading

darthrellimnad Apr 28, 2018 •

edited

Loading

craig0990 commented May 2, 2018 •

edited

Loading

darthrellimnad commented May 3, 2018 •

edited

Loading