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

Should we clear errors on request actions? #424

Open
klis87 opened this issue Jan 2, 2021 · 7 comments
Open

Should we clear errors on request actions? #424

klis87 opened this issue Jan 2, 2021 · 7 comments

Comments

@klis87
Copy link
Owner

klis87 commented Jan 2, 2021

Right now we do it, but I feel we shouldn't. We don't clear data, so I think we shouldn't error too. It would be cleared anyway on success, and the user will always have a choice whether to render error during loading new request or not. Right now error is gone on loading, so displaying it then without copying error as local state is not possible.

@Flixbox
Copy link

Flixbox commented Jan 5, 2021

We would agree with that sentiment. We're currently facing the problem that our server returns a 500 and the JS only sees "Internal Server Error", while the server actually delivers a useful response with a proper error object. We currently have no easy way of inspecting that error or displaying it to the user.

image

@klis87
Copy link
Owner Author

klis87 commented Jan 5, 2021

@Flixbox what driver do you use? Did you try console.log this error in the app? as tools might not display it properly due to serializing issues. Anyway, is it related to this issue? If not, please create a dedicated one so this would be only about clearing error on request :)

@klis87
Copy link
Owner Author

klis87 commented Jan 5, 2021

my guess is you use fetch driver, indeed we might have a small hole here, but status code 500 is not used often for predictable errors, 500+ usually means network error, random server failure and so on, if my assumptions are correct, just switching to 4xx status code will fix this problem

@Flixbox
Copy link

Flixbox commented Jan 5, 2021

I see now that your original post is not related to my issue at all. I will do more troubleshooting and open a new issue as needed.

@iwan-uschka
Copy link

iwan-uschka commented Jan 17, 2021

I don't have any use case in mind particularly where error is still needed on (re)load which would result in "keep the current implementation as it is" but indeed i can see the benefit of keeping error until success which maybe helps someone.

This is a breaking change (if an implementation explicitly depends on how error behaves).

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2021

@iwan-uschka It would be needed if you wanted to show error still during sth is refetched. This really depends on the app and use-case, but the thing is, changing this will just give this extra flexibility.

Agreed this is a breaking change, so probably this will go to next major version, not before. I could add it now with an extra config option, but I don't feel like this is important enough to justify it.

@iwan-uschka
Copy link

...extra flexibility

Agree.

...this is important enough to justify it

Agree.

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

No branches or pull requests

3 participants