[김예인] 스프린트 미션 5#68
Hidden character warning
Conversation
devbini
left a comment
devbini
left a comment
There was a problem hiding this comment.
🎉 고생 많으셨습니다!
아마 첫 Express/MongoDB API 구현이었을텐데, 전반적으로 큰 문제는 없던 것 같아요.
프론트에서는 커스텀 훅을 적절히 사용하고 있기도 하고, 등록 폼의 touched 상태와 에러 메시지 같은 놓치기 쉬운 세부적인 내용들을 하나씩 처리한 점도 눈에 잘 보였네요.
서버에서도 모델을 분리와 응답을 지정된 형태로 맞춘 덕분에 프론트 페이지네이션과 연결하기 쉬운 구조가 되었습니다. 이런 가시성 좋고 심플한 구조로 캡슐화하는 것도 좋은 능력 중 하나이니, 지금처럼만 쭉 이어가도 좋은 개발 코드가 나올 것 같아요.
그래서.. 전체적으로 화면과 API를 이어보는건 잘 되어있으니... 이제는 프론트와 서버간 통신에 사용되는 구조와 CSS 영향 범위처럼 사소한 제약과 계약? 사항들을 정리하면 다음 작업에서 디버깅 하는 시간도, 유지보수하며 코드를 이해하고 수정하는 시간도 많이 줄어들 것 같습니다.
sprint 6을 먼저 리뷰 해 버려서, sprint 6에서 나온 리뷰 사항들은 여기에 중복으로 적지는 않았어요. 참고 해 주세요! :D
너무나 고생 많이 하셨습니다!
파이팅이에요 🤗
| .sort(sortOption) | ||
| .skip(offset) | ||
| .limit(limit) | ||
| .select('_id name price favoriteCount createdAt') |
There was a problem hiding this comment.
🚩 Check Point
카드가 사용할 이미지 필드가 response에 없는 것 같아요.
FE의 ProductCard는 product.images?.[0]를 먼저 확인하고 없으면 기본 이미지를 보여주고 있어서 아마 문제는 없는데 (실 동작 자체에는...)
서버 목록 응답에는 이미지 관련 필드가 다 빠져 있는 것 같아요!.
요구사항상 기본 이미지를 보여주는 처리는 잘 되어 있지만... 나중에 이미지가 있는 상품을 저장해도 목록에서는 계속 기본 이미지만 보일 가능성이 있네요ㅎㅎ
| const offset = Number(req.query.offset) || 0 | ||
| const limit = Number(req.query.limit) || 10 | ||
| const keyword = req.query.keyword || '' | ||
| const orderBy = req.query.orderBy || 'recent' |
There was a problem hiding this comment.
☕ Thinking
sprint 6 리뷰에도 남겼듯,
정상적인 상황에서는 문제가 없지만, 실제 환경에서는 어떤 문제가 나올지 몰라요.
예를 들어- limit 데이터가 음수로 들어간다거나, 비정상적으로 엄청 큰 데이터가 들어간다던가...
Math 라이브러리를 활용하면 아마 상/하 제한을 걸 수 있을텐데, 서버 리소스 보호를 생각하는 방법이 하나 더 있지 않을까 싶어 남겨둡니다ㅎㅎ
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }, [orderBy, keyword, page, pageSize]); //의존성 배열에 넣기 |
There was a problem hiding this comment.
☕ Thinking
의존성 규칙에 따라.... 상품 목록 요청이 여러 번 수행됐을 때 최신 데이터가 덮어씌워질 위험이 있어보이긴 하네요.
예를 들면..
- 사용자가 검색어를 빠르게 입력하거나
- 화면 크기 변경으로 pageSize가 바뀌거나
이런 경우 요청이 여러 개 날아갈 수 있어 보여요. 이때... 늦게 끝난 이전 요청이 setProducts, setTotalCount를 실행하면 화면에는 과거 데이터가 보일 수 있어요.
요청마다 id를 두고 가장 마지막 요청만 상태를 반영하도록 처리하는 것도 방법이고, 인터벌을 주는 등 다양한 방법이 있으니.. 안정성 향상을 위해 한번 생각 해 보죠ㅎㅎ
| @@ -0,0 +1,40 @@ | |||
| const BASE_URL = 'http://localhost:3000' | |||
There was a problem hiding this comment.
☕ Thinking
그 아래 보니까 axios.js와 이 곳 두 곳에 BASE_URL이 동일하게 선언이 되어있는 것 같아요.
이렇게 되면.. API 주소를 바꿀 때 두 파일을 모두 수정해야 합니다. 나중에 배포 주소나 프록시 설정이 바뀌면 한쪽만 수정되어 버그가 생길 수 있습니다. (휴먼 에러는 생각보다 자주 일어나요!)
공통 API_BASE_URL 상수를 만들어 함께 쓰면 유지보수가 더 쉬워지지 않을까요! 🤗
| * { | ||
| box-sizing: border-box; | ||
| } | ||
|
|
||
| body { | ||
| margin: 0; | ||
| font-family: Pretendard, -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif; | ||
| color: #374151; | ||
| } | ||
|
|
||
| a { | ||
| text-decoration: none; | ||
| } |
There was a problem hiding this comment.
☕ Thinking
React에서 css는 해당 컴포넌트에만 스코프가 생기는 게 아니라 전역으로 적용됩니다.
무슨 말이냐면, 이 CSS를 로드할 때 랜딩 페이지 스타일 뿐 만 아니라 등록 페이지의 링크나 body 기본 스타일에도 영향이 갈 수 있어요.
그래서.. 이런 a나 body, * 등의 전역 리셋 선택자는 index.css 같은 공통 파일에 모아서 관리하는 게 일반적이랍니다.
랜딩 전용 스타일은 .landingPage a처럼 페이지 기반 루트 클래스를 만들어 관리해서 스타일 충돌을 줄일 수 있습니다.
2 participants
기본 요구사항
공통
프론트엔드 구현 요구사항
랜딩 페이지
중고마켓 페이지
상품 등록 페이지
백엔드 구현 요구사항
중고마켓
심화 요구사항
배포 링크 : https://one4-sprint-mission-fe-20v4.onrender.com