[1단계 - 영화 목록 불러오기] 가브리엘(윤주현) 미션 제출합니다. #14
Conversation
Co-authored-by: Gabriel Ju Hyun, Yoon <gabrielyoon7@gmail.com>
copy-webpack-plugin을 이용한 이미지 로드 문제 해결
함수명 변경, 전체적인 구조 변경
+ 결과 없음 페이지 css 수정
|
미션 설계하는 입장에서 저도 당연히 이미지는 제공될 것이라고 생각했는데 |
오류 방지 이미지는 실제 TMDB 사이트에서 가져왔습니다..ㅎㅎ 이번 미션에서는 영화 리스트를 비우고 fetching한 데이터를 새로 그리는 과정에서 Proxy가 너무 과도하게 개입해서 깜빡임 현상이 있더라구요ㅠㅠ 편했던 기능이라고 생각해서 다시 활용하고 싶었지만, 이번 미션에는 맞지 않는 기술이라고 생각해서 과감하게 포기하고 다른 방법을 택했습니다...! 응원 감사합니다! |
There was a problem hiding this comment.
안녕하세요 가브리엘~ 이번 미션 리뷰어를 맡게된 카일이라고 합니다. 코드 관련 리뷰는 코멘트 확인 부탁드리고 여기서는 전반적인 피드백 남기도록 하겠습니다.
초기에는 웹 컴포넌트로 구현하면서 구조를 조금 더 간단하게 구성할 수 있을 것으로 기대하였으나, 인기 영화 목록과 특정 키워드 검색 결과를 같은 코드에서 공존하게 하려는 시도에서 다소 복잡해진 경향이 있다고 생각합니다.
비슷한 로직을 하나의 함수에서 모두 처리하려고 해서 그런건 아닐까 생각합니다. 인기 영화와 검색된 영화의 공통점은 '영화'라는 데이터를 받아온다, 정도가 있을 것 같은데요. 컴포넌트든 api든 최소한의 공통적인 부분만 추출하여 세부 정보를 가진 도메인에서 호출하는 방식으로 구현해보면 좋을 것 같습니다.
적절한 예시가 될지는 모르겠으나 참고해보면 좋을 것 같습니다!
// as-is
A input | B input -> f -> A output | B output
// to-be
sharedf // 공통 부분을 추출한 함수
A input -> Af(shared f) -> A output
B input -> Bf(shared f) -> B output
cypress/support/commands.ts
Outdated
There was a problem hiding this comment.
cypress로 초기 세팅 시 생성되는 파일인데, 삭제하면 오류가 발생합니다 ㅠㅠ
Error: Webpack Compilation Error
./cypress/support/e2e.ts
Module build failed
There was a problem hiding this comment.
./cypress/support/e2e.ts에서 삭제된 파일을 import 해서 그런게 아닐까요 ㅎㅎ
There was a problem hiding this comment.
step2에서 재검토 해보겠습니다!
cypress/fixtures/example.json
Outdated
There was a problem hiding this comment.
cypress로 초기 세팅 시 생성되는 파일인데, 삭제하면 오류가 발생합니다 ㅠㅠ
Error: Webpack Compilation Error
./cypress/support/e2e.ts
Module build failed
There was a problem hiding this comment.
example.json만 삭제하고 번들링을해도 해당 오류가 발생하는 거 맞을까요?
There was a problem hiding this comment.
step2에서 재검토 해보겠습니다!
src/utils/eventListener.ts
Outdated
| listener: (event: Event) => void | ||
| ) => { | ||
| target?.addEventListener(type, (event) => { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
카일 말씀을 듣고 곰곰이 생각해보니깐 form의 제출이 정말 필요한 부분에서는 대응이 어려운 것 같습니다.
export const executeEventListener = (
target: Element,
options: {
type: string,
prevent: boolean
},
listener: (event: Event) => void
) => {
target.addEventListener(options.type, (event) => {
if (options.prevent) {
event.preventDefault();
}
listener(event);
});
};
그래서 문제를 해결하기 위해
event type과 prevent 여부를 추가로 줄 수 있는 형태로 수정했습니다~
There was a problem hiding this comment.
addEventListener 안에서 분기처리가 필요할 때마다 options의 인자 수도 늘어날 수 있을 것 같은데 그냥 addEventListener자체를 써도되지 않을까요? executeEventListener가 native api를 사용했을 때에 비해 어떤 장점을 가져갈 수 있는지 잘 와닿지 않는 것 같아서요..!
There was a problem hiding this comment.
step2에서 제거할 수 있도록 하겠습니다.
src/domain/movieStore.ts
Outdated
| import { updateMovies } from "../components/movieListHandler"; | ||
| import { IMovie } from "../type"; | ||
|
|
||
| export const movieStore = { |
There was a problem hiding this comment.
상태를 관리해야 한다면 클래스로 선언하시는 것을 권장드립니다. 현재는 movies가 public하게 노출되어 있어 어디서든 수정이 가능한데 객체 지향 관점에서 적절한 방향은 아닌 것 같습니다.
There was a problem hiding this comment.
갑자기 깊은 깨달음을 얻었습니다.
자바스크립트에서는 싱글턴을 언제 쓸 수 있을까 고민스러웠는데 이제야 활용할 수 있게 되었네요.
피드백 반영했습니다!
src/components/headerHandler.ts
Outdated
There was a problem hiding this comment.
handler와 대응되는 컴포넌트는 한 파일 안에서 선언하는 것이 코드 파악에 용이하지 않을까요? 어떤 관점으로 분리하신 건지 알고 싶습니다~!
There was a problem hiding this comment.
어떤 컴포넌트를 조작하는 함수를 외부로 분리하여 대응할 수 있도록 했습니다.
웹 컴포넌트는 뷰의 역할만 하고, 핸들러는 해당 컴포넌트를 조작하는 형태로 분리했습니다.
말씀해주신 것 처럼 한 파일에서 선언하게 되면 메서드가 너무 많아질 수도 있다고 생각했습니다.
파일을 합치지 않고, 폴더를 구분하여 연관있는 파일끼리 묶을 수 있도록 개선했습니다!
There was a problem hiding this comment.
넵 변경하신 것처럼 컴포넌트를 폴더 단위로 묶어 로직과 뷰를 파일로 분리하는 방향은 적절해보입니다! 다만 컴포넌트 내에 책임 단위로 나눈 파일이 많아질 경우를 고려해서 다양한 패턴을 시도해보면 좋겠네요!
- 장 - 한 파일에 비교적 적은 코드를 둘 수 있음
- 단 - 컴포넌트를 만들 때마다 생성할 보일러 플레이트 증가, 코드 유지보수 시 파일 간 이동이 잦음
|
안녕하세요? 이번 피드백에서 저를 가장 고민하게 만든 부분은 영화 데이터를 요청하는 부분이었습니다. �감사합니다! |
src/domain/movieApi.ts
Outdated
| @@ -2,12 +2,19 @@ import { removeMoreButton } from "../components/movieListHandler"; | |||
| import { Movie } from "../type"; | |||
| import { movieStore } from "./movieStore"; | |||
|
|
|||
| interface MovieApiResponse { | |||
| interface MovieResult { | |||
There was a problem hiding this comment.
클라이언트에서 사용할 인터페이스의 필드는 camelCase로 작성하는 것이 자연스러울 것 같습니다~ (이번 미션에는 고칠 필요없음)
There was a problem hiding this comment.
수신받은 영화 정보의 필드명이 results임을 고려하여 MovieResult으로 명명했으나, 조금 더 명확한 이름인 MovieApiResponse으로 변경했습니다.
(해당 인터페이스의 필드는 수신받은 api 데이터의 형태 그대로입니다 ㅠㅠ)
| const response = await fetch(url); | ||
| catchError(response.status); | ||
| try { | ||
| const response = await fetch(url).then((data) => data.json()); |
There was a problem hiding this comment.
await와 then 체이닝을 같이 쓰는게 어색해 보입니다! 하나만 사용해보는 게 어떨까요
const response = await fetch(url)
const { results, total_pages } = await response.json();
배포 페이지(오류 수정 완료)
프로그램 소개
안녕하세요? 이번 미션에서 중점적으로 사용한 기술과 기능 그리고 설계 원칙에 대해 말씀드리겠습니다.
컨트롤러 없음
customElement 사용
도메인 로직
movieApi.ts
movieStore.ts
실패한 것
숨겨진 기능
아쉬운 점
인기 영화 목록과특정 키워드 검색 결과를 같은 코드에서 공존하게 하려는 시도에서 다소 복잡해진 경향이 있다고 생각합니다.