-
Notifications
You must be signed in to change notification settings - Fork 38
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
[1단계 - 상품 목록] 버건디(전태헌) 미션 제출합니다. #3
Conversation
Co-authored-by: Pakxe <pakxe@users.noreply.github.com> Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: Pakxe <pakxe@users.noreply.github.com> Co-authored-by: pakxe <pigkill40@naver.com> Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com> Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com> Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com> Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: Pakxe <pakxe@users.noreply.github.com> Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: Pakxe <pakxe@users.noreply.github.com> Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
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.
이번에 주된 학습 목표 중 하나가
✔️ MSW를 사용하여 API 요청을 모킹하고 개발할 수 있다.
였는데요.
msw 내에서 페이지네이션과 카테고리 정렬 및 오름 차순 및 내림차순 관련 로직이 있다보니 msw 내부 코드가 굉장히 무거워지는 듯한 느낌이 들었습니다 🥲
그러다보니까 msw 내부에서 사용하는 코드를 위한 테스트 코드도 작성을 해야하는가 ?에 대한 의문점이 생겼었는데요.
그것이 아니라면, 각각의 요청 경우의 수에 맞춰서 각각 MockData를 만들어주어서 리턴해주는 방식도 있을거 같았는데, 이러면 경우의수가 많아지면 질수록, 더 힘들어지지 않나? 싶었습니다.
예를 들어
// 카테고리는 음식이고, 페이지는 2이며, 오름차순인 데이터
// 카테고리는 전체이고, 페이지는 1이며, 가격은 내림차순인 데이터
// 그 외 데이터
// 그 외 데이터
이런식으로 각각 경우의 수에 대응하는 데이터들이 다 생겨야 하는 것보다, msw 핸들러 내부에서 계산 로직을 작성해주는 것이 낫겠다는 판단을 했습니다!
하지만 ,동시에 이 계산 로직은 백엔드 측에서 담당해야하는 로직이 아닐까? 싶어서, 비용적으로 많이 드는 행위인거 같다고도 생각이 들었어요.
제가 생각하고 있는 부분중에 놓친 부분이나, 카일께서는 어떻게 생각하실지 한번 여쭙고 싶습니다.
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.
msw를 사용하는 목적성에 대해 고려해보면 좋을 거 같아요. 실제 프로덕션에 영향을 미치는 코드를 제외한 모든 코드는 '제품 개발의 효율성을 올리기 위해' 사용하는 목적이 가장 크다고 생각합니다. 하지만 이러한 목적을 간과하다보면, 완벽한 테스트 커버리지를 위해 오히려 제품 개발의 속도를 저하시키면서 주객전도 되기 쉬울 것 같습니다.
다음과 같은 상황에 몰입해보면 어떨까 싶어요.
- 개발을 하며 api를 호출하여 앱의 다음 동작이 정상적으로 동작하는지 확인해야 함
- 협업을 하고 있는 백엔드 측에서 아직 api가 준비되지 않은 상황
백엔드에 의존성이 걸려있는 현재 상황에서 '어떻게 하면 개발을 가장 빠르게 진행할 수 있을지'에 대한 측면으로 고민한 뒤에 버건디의 의견을 말씀해주시면 좋을 거 같네요~
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.
어떻게 하면 개발을 가장 빠르게 진행할 수 있을지
에 대해서 말씀 주신 덕분에 한번 생각해볼수 있었습니다!
이번에 페어와 함께 진행하면서는 어떠한 쿼리 파람이 오더라도 그에 응당하는 데이터를 보내주는 식으로 msw 내에 서버로직을 작성했었는데요..!
사실 로직을 작성하면서도, 이건 서버쪽의 로직이니 수정사항이 생기더라도 불가항력적으로 그에 맞게 프론트에서 대응해줄수밖에 없을거 같다고 생각이 들었습니다.
그러다보니까, 정확도는 올라가도, 비용적으로 이게 맞나? 라는 의구심도 들었던거 같아요.
다시 한번 고민해보면서, 최대한 빠른 피드백을 받을수 있는 방식으로 테스트 코드 수정해보았습니다.
짚어주셔서 감사합니다 🙇
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.
넵, 좋은 방향이라고 생각합니다 :) 실제 현업에서는 기획 -> 디자인, 백엔드 -> 프론트엔드 순으로 작업 의존성이 걸려있는 부분이 대다수이기 때문에 백엔드 쪽 기획이나 로직이 변경될 여지도 고려해보면 케이스 하나하나마다 mock api를 작성해두는 방식을 그렇게 권장할 수는 없을 거 같아요. 때문에 UX의 큰 범주에서 가장 중요한 부분을 위주로 테스트를 해보면 좋을 거 같습니다.
src/mocks/handlers.ts
Outdated
}); | ||
}), | ||
|
||
// ⛔️ async 추가로 인한 이펙트 확인 안됨. ⛔️ |
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.
후반부에 급하게 핸들러 코드를 작성하다 보니, 주석이 삭제가 안되어있는데요.
수정사항 반영하면서 동시에 반영하도록 하겠습니다. 죄송합니다 🙇
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.
안녕하세요 버건디~ 다시 보니 반갑네요 ㅎㅎ 세부 코멘트 남겨두었고 질문에 대한 답변도 달아두었으니 확인 후 재요청 주세요!
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.
일반 .ts
파일이 아닌 선언파일에 타입을 선언하신 의도가 궁금하네요~
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.
ts파일 같은 경우, 실질적으로 타입스크립트 코드를 작성하고, 컴파일러에 의해서 자바스크립트로 변환 되는것으로 알고 있는데요!
d.ts 같은 경우에는 자바스크립트로 변환되지 않고 타입 정보(?) 만 담고 있는 것으로 알고 있습니다.
d.ts 의 d가 definition, 정의를 의미한다고 알고 있는데요.
실제로 타입에 관한 정의들을 d.ts에 몰아서 작성해줌으로써 분리를 해왔었습니다.
제가 잘못 알고 있거나, 더 추가로 알아야할 점이 있다면 말씀주시면 감사하겠습니다!
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.
대부분 말씀 주신 부분은 모두 맞습니다. 다만 선언 파일의 사용 목적은 주로 외부 라이브러리와 연관되어 있는데요, 예를 들자면
- 어떠한 라이브러리를 만들고 있으며, 이에 대한 타입 정보를 외부 사용처에 제공할 때 (e.g. package.json에 선언해둔
@types/**
의존성) - 외부 의존성을 프로젝트 내에서 사용하면서 프로젝트 내에서 타입 호환성을 맞추고자 할 때
이러한 맥락으로 *.d.ts
파일의 영향 범위는 전역이기 때문에 프로젝트 내부의 타입을 선언하여 사용하기에는 네이밍 충돌 우려가 있을 듯 합니다.
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.
*.d.ts 파일의 영향 범위는 전역
몰랐던 사실인데 짚어주셔서 감사합니다 :)
src/pages/ProductListPage.tsx
Outdated
|
||
let bottom = useRef(null); | ||
|
||
const onIntersect = ([entry]: IntersectionObserverEntry[]) => |
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.
onIntersect
호출 시 앞선 조건이 false
라면 이 메서드는 false
값을 반환할 것 같은데, 명확하게 무언가를 반환할 함수가 아니라면 의도하지 않은 값이 반환되지 않도록 처리해두어야 예상치 못한 사이드 이펙트를 방지할 수 있습니다.
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.
짚어주셔서 감사합니다 카일!
사실 페이지 내부에서 감지하는 ref가 존재하는것도 조금은 어색하지 않나? 생각이 들었습니다.
또한 말씀처럼 딱히 무언가를 반환하는 로직이 아닌것도 마음에 걸렸었는데, 말씀 주신 의도하지 않은 값이 반환되지 않도록 처리해두어야 예상치 못한 사이드 이펙트를 방지
이 이유가 와닿았습니다.
아예 InfiniteScrollObserver
라는 컴포넌트로 관심사를 분리해보았어요!
다만 이 InfiniteScrollObserver
내부에 임의로 보이지 않는 div
에 ref 값을 주어서 스크롤을 감지하고 있는 상태인데요.
레벨1 영화미션에서 무한스크롤을 감지할때는 아예 li 태그의 마지막 아이템을 감지하도록 해서 추가적인 데이터 페칭을 했었습니다!
이번에도 ProductItem 마지막 요소에 ref를 주어야할까 ? 라는 생각을 했는데, 이렇게 되면 depth 도 깊어질뿐더러 (Page 컴포넌트 => List 컴포넌트 => Item 컴포넌트)
forwardRef로 감싸주기도 해야하고.. 또 ref 를 외부에서 선언해주어야하는 단점(관심사 분리가 잘 되지 않는다.)이 생기더라구요.
그래서 불가피하게 아예 임의의 div 태그를 만들어주었는데, 위치 감지만을 위해 쓸데없는 태그가 하나 만들어진것 아닌가(비용이 더 든건 아닌가)? 라는 의구심도 듭니다.
카일은 혹시 이에 대해서 어떻게 생각하실지 여쭤보고 싶습니다! 🙇
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.
태그 하나 정도는 비용적으로 크게 고민할 정도는 아니라고 생각합니다. ㅎㅎ 오히려 ref에 대한 책임을 사용처 vs 내부 중 어느 곳에 두어야 할지 고민을 해봐야 할 듯 해요. 대개 책임을 사용처에 두는 것은 여러 variation에 대처 가능하도록 커스터마이징을 용이하게 하는 목적이 크지만, 내부에 두는 경우는 적용 방식에 크게 차이가 없을 것으로 예상하기 때문에 사용처에서 이에 대해 몰라도 되므로 DX를 높이는 효과가 있을 것 같습니다.
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
Co-authored-by: pakxe <pigkill40@naver.com>
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.
Dropdown을 컴파운드 패턴으로 관리해보고 싶어서 시도해보았는데요..!
아직 UI상 에러가 계속 발생해서 일단 커밋만 올려놓은 상황입니다.. 🥲
step2때 고쳐서 수정해보도록 하겠습니다!
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.
엄연히 보자면, production에 나갈 준비가 되지 않은 코드는 병합될 PR에 포함되면 안된다고 생각합니다! 완전한 동작을 한다는 전제로 2단계 PR에 올려주시는 게 좋았을 듯 싶습니다.
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.
리베이스 과정에서 파일 자체가 손실 될까 싶어서 일단 올려놓은건데, 아무래도 카일 말씀이 맞는거 같습니다. 🥲
다음에도 이런 일이 있다면 유의하겠습니다!
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.
안녕하세요 버건디~ 리뷰 수정사항 모두 확인하였고 추가적으로 필요한 코멘트는 남겨두었으니 확인 부탁드립니다! 해당 미션은 여기서 이만 머지하도록 하고 다음 미션에서 뵐게요! 고생 많으셨습니다.
배포주소
안녕하세요 카일~ Lv1 로또때 뵈었었는데, 이번에 다시 뵙게 되었네요 : ) 잘지내셨나요
배포 된 주소를 확인하시려면, 안전하지 않은 컨텐츠 허용을 해주셔야지만 확인이 가능합니다..! 양해 부탁드립니다 🥲
질문 드릴 부분은 따로 코드 코멘트로 남겨놓겠습니다!
마지막 Lv2 미션 잘부탁드립니다 🙇