Replace $http with axios #4497
Replace $http with axios #4497
Conversation
kravets-levko
added
Frontend
Frontend: React
There was a problem hiding this comment.
@kravets-levko I've worked in a few services (so far, the simplest ones).
I was thinking about keeping this Object-actions pattern for those since they don't need any more than that. For the complex ones (with large class services), I'll see how they behave tomorrow.
| function QuerySnippetService($resource) { | ||
| const resource = $resource("api/query_snippets/:id", { id: "@id" }); | ||
| resource.prototype.getSnippet = function getSnippet() { | ||
| class QuerySnippet { |
There was a problem hiding this comment.
Example of simple class in a service (this one is read-only, so has no issues).
I could rewrite the getSnippet in a non-class way, but wanted to show a simple class in here (for other services it's the same base idea, but a lot more complex)
| @@ -36,7 +37,7 @@ function CreateDashboardDialog({ dialog }) { | |||
| if (name !== "") { | |||
| setSaveInProgress(true); | |||
|
|
|||
| $http.post("api/dashboards", { name }).then(({ data }) => { | |||
| axios.post("api/dashboards", { name }).then(({ data }) => { | |||
There was a problem hiding this comment.
I think that things like this should be moved to services (e.g. have a method called updateDashboard of whatever) - there should be no direct HTTP calls in components. There are a lot of similar places in other components as well
There was a problem hiding this comment.
You're right. It was wrong to use $http all over. But wherever things are just working, we should keep things as they were (except for adopting axios so we can drop $http). In parallel, let's decide on a pattern we want to use for our API calls/resources, that will be more inline with how React works (returning immutable objects for example). Any suggestions for such pattern? Let's discuss in the issue.
Wherever things are not working as is or we have to jump through hoops to make it work, we can start adopting the new pattern. The rest we will update when we are done with the React migration.
| const QuerySnippetService = { | ||
| query: () => axios.get("api/query_snippets").then(data => map(data, getQuerySnippet)), | ||
| create: data => axios.post("api/query_snippets", data).then(getQuerySnippet), | ||
| save: data => axios.post(`api/query_snippets/${data.id}`, data).then(getQuerySnippet), |
There was a problem hiding this comment.
Interesting solution 👍 But you should keep in mind that some API calls do not return a full updated instance (for performance reasons), but only updated fields. I believe, only query and dashboard API do so, but anyway. Also, in some places we started using a (sort of) pattern: we implement a method that updates some instance; it accepts instance and a list of updates, makes a request to backend, then creates a copy of instance and picks from server's response only fields passed in arguments (or few others, like version for queries and dashboard). I think we should not try to mimic Angular resource here and try to use this pattern where possible.
There was a problem hiding this comment.
I think the simpler ones don't need much more than this.
My thoughts for this part was to try to keep every response returning a new valid object and minimizing the changes in code. The update pattern is interesting, but also depends on each resource... Wdyt of simplifying it here and postpone this update to when we define a standard for an ideal api resource layer?
It's not "exactly" an Angular resource mimic 😅, the class in this case was more to show how it could be done, only the API requests change the object, so in this case it's not an issue :). Angular resources also had the api methods in its prototype ($save, $delete, etc), those are quickly changeable, I'm more concerned about the ones that have lots of extra stuff in the prototype (I'll share comments when I reach them 🙂)
| export const axios = axiosLib.create(); | ||
|
|
||
| const getData = ({ data }) => data; | ||
| const getResponse = ({ response }) => Promise.reject(response); |
There was a problem hiding this comment.
Is this needed to get a response object instead of error when request fails? If yes - I don't think it's so good idea. Angular's $http had similar behavior, and that's why we needed that PromiseRejectionError - to convert response to a Error instance, because in terms of error handling it's much more convenient if all promises are rejected with Error objects - global error handler does not need to perform additional checks, and if you handle them locally in component - you alread know what type of error you have and can, say, get error.response there.
There was a problem hiding this comment.
I mostly returned the response like this because it was the behavior $http had and I wanted to minimize the risks -- I suppose we hardly ever test errors in the application (unlike the success flow), it's a point to focus for future and add tests to alternative/error paths as well.
Regarding the global promise handler, I agree, it's easy to make sure it's indeed an error and show an error page in the end when the response is an Error instance. I think we can try to remove it when handling errors globally (not sure if you're doing this in #4525, I still need to sync myself to everything that's in there again).
In brief, I believe this is correct way to do it, but my fear is that changing this may cause issues like unhandled promises showing errors where you didn't want them and error notifications not showing what they should.
There was a problem hiding this comment.
Got it 👍 Let's merge it, and revisit it later if needed.
What type of PR is this? (check all applicable)
Description
Replace $http and other related angular dependencies with axios ($resource and $q)
Related Tickets & Documents
#3071
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
--