-
Notifications
You must be signed in to change notification settings - Fork 15
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
[조형민] sprint4 #30
base: basic-조형민
Are you sure you want to change the base?
The head ref may contain hidden characters: "basic-\uC870\uD615\uBBFC-sprint4"
[조형민] sprint4 #30
Conversation
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.
형민님 이번 스프린트 고생하셨습니다!
작성해주신 코드와 커밋들을 보니 경험도 많으시고 개발을 잘 하시는 것 같아요
커밋도 깔끔하게 적기 힘든데, 더군다나 컨벤셔널 커밋들까지
BREAK CHANGE를 여기서 처음보는것 같아요 ㅎㅎ
고칠것은 사실 거의 없고 공통 로직 따로 빼는것 정도 코멘트 남겨놨습니다!
나중에 한번 확인해주세요~
수고하셨습니다!
import axios from "axios"; | ||
|
||
const instance = axios.create({ | ||
baseURL: "https://sprint-mission-api.vercel.app/", |
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.
axios 로 작성하셨다니 좋습니다~ 다만 baseUrl 에 마지막에 슬래시는 없어도 될것 같습니다!
아래쪽에 인스턴스에서 get 등을 호출할때 "/articles" 이런식으로 호출되다보니
https://sprint-mission-api.vercel.app//articles 이런식으로 들어갑니다. 동작은 하지만 그래도 슬래시 하나 빼주는게 더 좋을것 같아요!
baseURL: https://sprint-mission-api.vercel.app
요런식으로요
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.
넵! 명심하겠습니다. 안 그래도 다음 미션할 때 base url에 / 했다가..계속 에러가 나서 한참을 헤맸었습니다 ㅠㅠ
); | ||
} | ||
|
||
/* fetch로 구현 연습 */ |
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.
오.. fetch로도 따로 구현하시고, axios로 작업하셧군요!
ArticleService.js
Outdated
.delete(`/articles/${id}`) | ||
.then((res) => console.log(res.data)) | ||
.catch((e) => | ||
console.log( |
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.
다 같은 에러 로깅을 하고 있는 것 같은데 요런것을 따로 함수로 빼면 더 간단해질듯 합니다.
function logError(e) {
console.log(`status: ${e.response.status}\nmessage: ${e.response.data.message}`)
}
//
instance.get(`/articles/${id}`)
.then((res) => console.log(res.ata))
.catch(logError);
요렇게 빼면 모든 catch문을 저렇게 변경할수 있습니다!
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.
우왓..이걸로도 엄청 깔끔해지네요.
@@ -0,0 +1,61 @@ | |||
import axios from "axios"; | |||
|
|||
const instance = axios.create({ |
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.
axios.create로 인스턴스까지 만드실줄 아신다면, 저는 axios 관련 js 를 만들어서 ArticleSerice, ProductService에서 같은 인스턴스로 빼볼듯 합니다! 중복된 코드 제거는 언제나 좋습니다 ㅎㅎ
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.
당장은 무슨 뜻인지 잘 모르겠지만 잘 생각해보겠습니닷! 😳
|
||
export async function getProductList(params = {}) { | ||
try { | ||
const res = await instance.get("/products", { params }); |
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.
오 여기는 async await 을 하셨군요 try catch로 잘 처리 된것 같습니다!
저는 개인적으로 async await + axios 를 많이 씁니다만 상황에따라 then도 많이 씁니다. fetch는 잘 안쓰는것 같네요 ㅎㅎ 다만 axios안쓰고 fetch만 쓰는곳들도 있다고 들었습니다. 고로 역시 다 알고 잘 써야하긴 해요! 그래도 axios, async await 를 좀더 추천드립니다. |
@paengdal 님 이거 conflict 들 해결해주시겠어요?? 머지하려고 합니다! |
아 제가 conflict를 배우긴 했지만 한번도 해본적이 없어서 ㅠㅠ |
아 저 아래 파일들이 아니군요! conflict를 확인할 수 있는 방법만 좀 부탁드립니다! |
뭣 때문에 충돌이 나는지, 어디서 내용을 보는지도 잘 모르겠네요. ㅠㅠ |
죄송한데 검색해서 이것저것 입력해보고는 있으나 잘 모르겠습니다. |
요구사항
기본 요구사항
공통
주요 변경사항
스크린샷
멘토에게