-
Notifications
You must be signed in to change notification settings - Fork 64
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
Api integration shop #111
base: develop
Are you sure you want to change the base?
Api integration shop #111
Conversation
try { | ||
const { | ||
data: { Products }, | ||
} = await axios.get(`${SOP_PRODUCT_BY_ID_ENDPOINT}/${id}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use axiosInstance over here. Since the base url will always remain same.
In utils we can have class that handles network request for the project and expose methods that will return api result. This will help in decoupling network calls from component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good , we can actually go 1 step further and make our own request handler function for this , that would work like a higher level api for multiple request types including handling of options building query string, i actually have worked on a similar kind of thing in my current project which works like a charm, let me know if you want me to go ahead with that , and we can discuss that further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes so it would be like wrapper function where we can give options as input and make network call.
addCartItem: PropTypes.func, | ||
addShopListItem: PropTypes.func, | ||
delCartItem: PropTypes.func, | ||
delShopListItem: PropTypes.func, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all these props are not required, then we should have these included in the defaultProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But should we have placeholders of functions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these functions props are required in this component, make it as .required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But a common track for component testing is to have two exports with and without redux's connect , just for testing props , and since functions are part of pre compiled code do we really need to make it is required?
// console.log(Products); | ||
return Products; | ||
} catch (error) { | ||
console.log(error.message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a way where we can let the user's on the screen know that there has been some error, rather than just console logging it. If I click on something which is performing an http
request, If there is an error, how will I know about it. What say @Kratika0907
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly what i was thinking , but for that we should better restructure the redux for handling and keeping track of network errors
// context.res.setHeader('Cache-Control', `max-age=${CACHE_MAX_AGE}`); | ||
|
||
const products = await getShopProducts(); | ||
// console.log(products); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT : Please remove this console statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
Hey @Akashdeep-Patra, Would you like to make changes as suggested and resolve merge conflicts? |
the end point mentioned in the code is of my local mock backend,(for the sake of testing), using Axios just because I use it allot,
suggestion: we are making a network call in the product details page because there is an endpoint to there, however, we are loading the products on the shop page, can put it in redux, would save us a network call?