-
Notifications
You must be signed in to change notification settings - Fork 34
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
Feature request: dispatch an _ERROR action if there was an API error #17
Comments
(Disclaimer – I don't know TypeScript, but this looks like a pretty vanilla JS change anyways) |
Oh and obvs I could do this in my reducer, but I think it's tidier to do all the main switching on actions. |
Quick bump on this @mikestead – thoughts? |
@shankiesan I can see the angle you're coming from but I'm not totally sure about this one. As we always dispatch a FSA you can add middleware to inspect all actions for errors. From there you could siphon off more specific actions as required. An aim here is to give some basic wiring but to also keep things light and not get too opinionated around how Redux has been setup. Not super easy, but was one of the reasons I went with FSA as it was well established. |
Fair enough @mikestead . As you say, adding middleware to inspect all actions for errors is a good idea. I'll run with that. Cheers |
The middleware/api actions in the official real world Redux example uses the https://github.com/reactjs/redux/blob/master/examples/real-world/src/actions/index.js Just my 2 cents worth :) |
Hey @mikestead I only just noticed @jedrichards comment. This is basically my feeling as well. I appreciate the FSA approach, but I think the idiomatic Redux approach would be more suitable. How about a CLI flag for the action type? Obviously this is a major change, but something like |
@shankie-san @jedrichards Thanks for the feedback. Yeah a flag to opt into it sounds good. I'll reopen the ticket. I'd probably want to keep the current default to be backwards compatible. Don't have time to look at this anytime soon I'm afraid. |
Yep that makes sense to leave the current as the default @mikestead I'd be happy to help add this when we've got a new project that we're going to base on OpenAPI. Maybe @jedrichards and I could take a look at it? Anyways yes let's leave it open in the hopes that one of us has a project that makes it worthwhile. |
You're going to love me @mikestead .
When I've used Redux before, one of the starters that I used had a really nice model, that was, when making AJAX requests, to dispatch
_REQUEST
,_SUCCESS
and_FAILURE
actions depending on what happened in the api service. Originally I thought that this was daft, but actually, you want to be able to handle errors in different kinds of requests differently.It looks like it should be a pretty easy change as you'll just have to add a
.catch()
on the dispatch around here – and dispatch a new action, suchMY_ACTION_NAME_ERROR
. If I'm not mistaken,makeFetchRequest
already returns afetch
promise, which should already reject if there's an error, which means that there shouldn't be too much working through the entire codebase.Are my assumptions above correct? If so, would you accept a PR for that?
The text was updated successfully, but these errors were encountered: