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

Why does GET handle errors but POST, PATCH and DELETE do not? #239

Open
geoidesic opened this issue Aug 9, 2020 · 7 comments
Open

Why does GET handle errors but POST, PATCH and DELETE do not? #239

geoidesic opened this issue Aug 9, 2020 · 7 comments

Comments

@geoidesic
Copy link

geoidesic commented Aug 9, 2020

I notice that when loading (i.e. GET) it always handles the error by catching the error and calling handleError. However for create and update it does not catch the errors. This makes it impossible to write generic code for error handling. Effectively one has to separate requests out according to method and then handle it differently depending on whether it's a GET request or not.

Please can you explain why there is this difference (one might say inconsistency) in approach?

@CodingItWrong
Copy link
Contributor

Vuex

I've thought about recording error status for create/update/delete requests but haven't been sure how best to handle it. Here's why:

On a given page I expect I'm generally doing one load operation, so the store has a single loading and error flag. I expect to use those for loading and error indicators in the UI displaying a list of records, for example.

  • We can't use the same single flags for write operations because that would result in a list loading indicator when a record was being saved.
  • Maybe we could have a single flag for write operations? But this would be overwritten if an app sent two write operations at once
  • Maybe we could have one flag for create, one for update, and one for delete. I'm not sure if that provides any benefit
  • Maybe we could somehow track a flag for each write operations you do. If we do that, it's not obvious how you would be able to look up the right flag for the right operation. At that point, relying on the settled state of the promise returned by the action might be simpler

Do you have thoughts on which of these approaches, or another, might work well?

@geoidesic
Copy link
Author

geoidesic commented Aug 18, 2020

Maybe this library shouldn't be doing error handling. Maybe it should just be collecting promises and returning them. I personally would prefer that. Yes you'd have to change your tutorial but I think the implementation as it stands is flawed. Rather keep this as a pure interface to JSONAPI spec and allow the app to handle errors. For the app to handle errors though it needs to receive all promises.

So any method in index.js should wrap calls to client as promises and return those promises. If there's more than one call to client then use Promise.all.

Alternately (for backwards compatibility) keep the GET mechanism as it is but certainly for the other methods it should collect and return all promises. Currently many of the actions return void, which is not conducive to error handling.

@geoidesic
Copy link
Author

  • At that point, relying on the settled state of the promise returned by the action might be simpler

I think that's probably the only way to go. Anything else makes the library inconsistent and limits it's use to some very specific cases.

@geoidesic
Copy link
Author

geoidesic commented Oct 15, 2020

Here are problems as it stands:

  1. Error handling is not coherent – handles differently for different request methods. This means you can't write general error or response handlers, which is bad for automation.
  2. If you're doing multiple requests to the same endpoint, you can't tell which failed, only that one failed.
  3. Writing errors to vuex is ok for declarative UI to respond to but the current approach swallows the response and errors, which means the app can't do anything with them if it wants to. This thwarts e.g. polling techniques, it also prevents any signalling or logging of requests and their success / failure that one might wish to do client-side (e.g. via IndexedDB) and it also really gets in the way of caching strategies.

@geoidesic
Copy link
Author

Your last point about somehow track a flag for each write operations you do. That's how I would approach it, but not just for write operations. My feeling is that there should be errors and cache stores.

Success for all requests (all request methods) on all stores are written to the cache store. The data tracked would be key value pairs.

  • key: the full request URL and body serialised via sha256 as a UUID.
  • val: timestamp

Errors for any request would be tracked to the errors store in similar format but also including the response and error data.

These two stores would be special in that they may or may not be something one wishes to persist to the back-end.

@geoidesic
Copy link
Author

geoidesic commented Oct 16, 2020

Although I think probably all of that should be implemented as an optional extension, not as core to @reststate. I think @reststate should just return promises and provide actions for calling mutations. I.e. Each action has some mutations it runs on success and some on failure. These groups should be wrapped as actions themselves. So that if my app gets an XHR failure but decides that the reason for that failure is that the server is offline and since we're generating UUID's locally it's ok for the mutations to run anyway, that I can still run the mutations easily without a lot of manual coding. Probably this applies reststate-client, not reststate-vuex.

Let me know what you think.

@CodingItWrong
Copy link
Contributor

FYI, this library will be unmaintained going forward.

If you need new features or fixes, I recommend forking the repo and making changes, or finding an alternate library that meets your needs.

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

No branches or pull requests

2 participants