Skip to content

Conversation

@usageness
Copy link

@usageness usageness commented Mar 11, 2022

[1단계 - 나만의 유튜브 강의실] 유세지(김용래) 미션 제출합니다.

아사님, 안녕하세요! 다시 만나게 되어 반갑습니다 😊
이번 미션에서는 외부 API를 사용하고, 비동기 작업들이 많이 이루어지는 등
이전보다 미션의 난이도가 조금 높아졌기 때문에 주어진 요구사항들을
안정적으로 구현하는데에 중점을 두고 진행했습니다.

미션을 진행하며 몇 가지 모호한 점들이 생겼는데,
이 부분은 아사님의 의견이 궁금하여 조금 정리해보았습니다.

🤔 궁금한 점

mock 데이터를 이용한 통신 테스트는 필수적인가요?

단위 테스트에서 API 통신을 담당하는 부분을 확인하다보니
fetch에 관한 테스트가 정상적으로 이루어지지 않는 것을 확인할 수 있었습니다.
jest-fetch-mock 와 같은 우회 방법을 이용하여 테스트 할 수는 있겠으나
mock data를 이용해 통신이 잘 이루어졌는지 확인하는 단순한 테스트를 한다면,
"꼭 필요한 테스트가 아닌, 테스트를 위한 테스트이지 않을까?" 하는 생각이 들었습니다.

페어와 이야기를 나누고 내린 결론은 굳이 필요하지 않다 였지만
저희의 경험이 부족하여 잘못된 결론을 내린 것 같아 확신이 들지 않네요 😥
아사님은 어떻게 생각하시나요?

주석 처리된 테스트는 js/__tests__/app.test.js 파일을 참조해주시면 감사하겠습니다!

다른 설계 방식은 없을까요?

image

현재 프로젝트의 구조는 이렇습니다.
YoutubeApp.js 에서 이벤트를 만들고 바인딩 하는 등 프로그램의 흐름을 관리하고,
DOM 관련 조작은 dom.js에 메소드를 만들어 모두 일임해 주었습니다.
사용자가 영상 데이터를 저장하는 로직들은 UserStorage.js로 분리하여 관리하도록 해주었습니다.

같은 개념을 가진 데이터들을 관리하는 책임은 모델로 나눠주고, 공통 함수는 utils로 옮기고, DOM에 관련한 함수를 분리하고...
지금까지 미션을 진행하며 구현한 코드들이 큰 틀에서 벗어나지 못하고 비슷하게 만들어지는 것 같아서
다른 설계 방식이나 개념을 적용해보고 싶은데, 어떻게 만들어주면 좋을지 떠오르지가 않네요 ㅠㅠ
아사님께서 이러한 설계에 관련된 새로운 인사이트를 알려주시면 감사하겠습니다!


🚩 구현한 기능 목록

  • 메인 화면에서 검색 버튼을 누르면 검색 모달창이 나타난다.
  • 유튜브 검색 API를 사용해 내가 보고 싶은 영상들을 검색할 수 있다.
    • 엔터키를 눌러 검색할 수 있다.
    • 검색 버튼을 클릭해 검색할 수 있다.
    • 디바운스를 사용하여 API호출을 최소화한다.
  • 데이터를 불러오는 동안 현재 데이터를 불러오는 중임을 skeleton UI로 보여준다.
  • 검색 결과가 없는 경우 결과 없음 이미지를 보여준다.
  • 최초 검색 결과는 10개까지만 보여준다.
    • 브라우저 스크롤 바를 끝까지 내려 그 다음 10개 아이템을 추가로 불러온다.
    • 스로틀를 사용하여 이벤트 콜백함수 호출을 최소화한다.
  • 내가 검색한 영상들의 JSON 데이터를 localStorage에 저장한다.
    • 저장 가능한 최대 동영상의 갯수는 100개이다.
  • 이미 저장된 영상이라면 저장 버튼이 보이지 않도록 한다.

테스트 요구사항

  • 단위 테스트를 Jest로 작성한다.
  • E2E 테스트를 Cypress로 작성한다.

배포

  • 실행 가능한 페이지에 접근할 수 있도록 github page 기능을 이용하고, 해당 링크를 PR과 README에 작성한다.
  • API key를 public repo에 올리지 않은 채로 데모 페이지를 배포하려면, 별도의 설정이 추가로 필요합니다.

결과물은 아래 링크에서 확인 가능합니다.
다시 한 번 잘 부탁드립니다! 😊

데모 페이지

kkojae91 added 30 commits March 8, 2022 15:14
- UserLibrary에 영상 데이터가 저장되는 기능 구현
- @babel/plugin-transform-runtime 설치
- @babel/runtime 설치
- reset.css 임포트 추가
- 테스트 코드: 검색버튼 클릭 추가
- html skeleton template -> templates.js 이동
- reset css 제거
- 테스트 코드 수정
- index.html에 no-result -> templates.js 로 이동
- file-loader 제거
- asset/resource 설정

- not_found 이미지 CSS 수정
- 단위 테스트: jest localstorage mock import
- e2e 테스트: cy.get(".video-item__save-button") => eq(0) 추가
- 목업 데이터 수정
- css hide 클래스 추가
- 내가 검색한 영상들 중 저장 버튼을 누르면 저장 버튼이 사라지는 기능 구현
- 저장 버튼을 누르면 로컬 스토리지에 videoId를 저장하는 기능 구현
- jest-fetch-mock 설정
- API 호출 부분 테스트.. -> 왜 해야하는지 잘 모르겠음.. (확인이 필요함)
- netlify 통신을 통해 API key 숨기는 기능 구현
- eslint max-lines-per-function 제거
- skeleton loading UI Scroll 이벤트에 반응하도록 수정
Copy link

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

세지님 안녕하세요 ㅎㅎ 다시 만나게 되어 반가워요!
1단계 요구사항 잘 구현해주셨네요.

mock 데이터에 대한 궁금하신점은 아래 코멘트에 남겨드렸고 두번째 질문에 대해서는 적용해보시고 싶으신 설계나 개념을 알려주시면 더 자세히 답변드릴 수 있을것같아요! 코멘트로 알려주세요 😄

스크린샷 2022-03-12 오후 8 04 49

  • 첫 리뷰때보다 함수형이나 브라우저 API에 익숙해지셔서 좋았어요 👍
  • API 요청횟수가 끝났을때 추가적으로 예외처리가 되었으면 좋겠어요.

아래에 몇가지 코멘트를 더 남겼는데 한번 확인해주세요~
새로 시도해보고 싶은 설계나 패턴이 있으시다면 step2에 가서 고치는 것 보단 지금 고치는게 낫다고 생각이 들긴한데 구조를 조금만 리팩터링해봐도 이전보다 깔끔해질거라 생각해요.

하지만 세지님이 도전해보고싶은 패턴이 있다면 한번 이야기해보고 변경해봐도 좋을 것 같아요.
좋은 주말 보내세요 세지님~ 🙇

Comment on lines +5 to +9
/**
* 실제 API를 호출하지 않고 Mock Data를 이용하여 테스트하는 케이스는 불필요하다고 생각되는데...
* 리뷰어님 생각은 어떠신지 궁금하네용 😥
* 실제 API 통신을 테스트 할 때는 어떻게 하는지 알 수 있을까요?
*/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 작성한 컴포넌트 내부에서 해당 api에 직접적으로 호출하고 있는 경우에는 api를 모킹하는 과정이 필요하다 생각해요.
해당 케이스를 커버하기 위한 대표적인 라이브러리로는 msw가 있어요.

위 케이스가 아니라면 굳이 테스트가 필요없을거라고 생각해요. 👀
컴포넌트내부에서 호출하지 않고 인자를 통해 데이터를 주입받으면 별도의 서버 모킹없이 더미데이터로 테스트할 수 있는 장점이 있다고 생각해요.
설계, 상황에 따라 적절하게(?) 선택해주시면 된다고 생각합니다 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api를 가공해야 할 필요가 있는 메서드가 생긴다면 msw와 같은 라이브러리를 사용하여 테스트해주면 되겠네요!
좋은 방법을 알려주셔서 감사합니다 😁

this.userStorage = userStorage;
}

bindEvents() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 분리할 필요가 있을까 싶어요.

외부에 노출될 메서드가 아닌 것 같은데 생성자에서 처리해도 괜찮지 않을까요?? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생성자 함수가 비대해지는걸 막기위해 분리하였어요!
bindEvents 함수는 private 키워드를 통해 외부로 노출되지 않도록 변경해주었습니다 😄

Comment on lines 25 to 27
setLocalStorage() {
localStorage.setItem("videos", JSON.stringify(this.storage));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

별도로 분리하지 않아도 괜찮을 것 같아요~

@@ -0,0 +1,28 @@
import { ERROR_MESSAGE, STORAGE_MAX_COUNT } from "./constants/constants";

export default class UserStorage {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UserStorage보다 VideoStorage가 더 알맞다고 생각해요.
필드나 메서드 네이밍도 한번 고민해보시는걸 추천드려요 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

파일과 메서드 네이밍도 조금 더 명확하게 변경하였습니다!

UserStorage.js -> VideoStorage.js
userStorage.test.js -> videoStorage.test.js
addStorage() -> addVideoData()

@@ -0,0 +1,69 @@
import { ALLOCATE_FOR_RENDER_PX, ERROR_MESSAGE } from "../constants/constants";
import { isEmpty } from "./utils";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 파일에서 너무 과도하게 분리한 함수들이 많아요.
이미 직관적이고 일반적으로 익숙한 메서드를 한번 더 함수로 랩핑하게되면 다른 개발자가 작성할 때 이미 익숙한 함수를 작성할 확률도 있고 이해하는데 함수를 한번 더 확인해봐야 하는 문제가 있어요.

정말 분리가 필요할지 생각해보시고 알려주셨으면 좋겠어요!

Copy link
Author

@usageness usageness Mar 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일관성을 생각해서 함수를 잘 나누어주었다고 생각했는데
저도 이번에 코드를 보면서 함수를 하나하나 다시 확인해보고 있더라구요... 😥
별도의 모듈 (searchModalView) 을 만들어서 책임을 나누어 주었습니다!

e668a6b

bindEventListener(
this.videoList,
"scroll",
throttle(this.onScrollVideoList, DELAY_TIME)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 미션에서 중요한 포인트라고 생각되는 throttle을 적용해주셨네요 👍
debounce에 대해서도 여유가 생긴다면 학습해주세요~

다른 크루들이 infinity scroll을 intersection observer을 구현한 경우도 있던데 추가적으로 공부해보면 좋을 것 같아요 👀
IE를 지원하는 경우가 아니라면 좋은 선택이라 생각해요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기회가 된다면 (되도록이면 step2 이내로...!) 적용시켜 보도록 할게요 :)
공부할만한 키워드 남겨주셔서 감사합니다 😁

expect(userStorage.isSavedVideoId(responseId)).toBe(true);
});

test("이미 저장된 videoId이면 false를 반환한다.", () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 명이 잘못되지 않았을까요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😥😥😥

@@ -0,0 +1,37 @@
import { isDuplicate, parsedDate, isEmpty } from "../utils/utils";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트의 케이스가 많은건 좋다고 생각하는데 의미있는 테스트인지는 생각해볼 필요가 있어보여요.
유틸리티가 많이 변경될 여지가 없다고 생각되기도 하구요. 테스트를 위한 테스트가 된 느낌입니다 😅
코멘트를 드린 것 처럼 의미있는 단위로 역할을 나눌 수 있게 모듈을 재구성해보면 좋겠어요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

직접 만든 코드는 꼭 테스트해야 한다고 생각했었는데
필요한 테스트가 무엇인지 생각해보는것도 중요할 것 같네요..!
다음 step에서는 테스트의 단위가 의미를 가지도록 조정해보겠습니다!

validateInput,
} from "./utils/dom";

export default class YoutubeApp {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YoutubeApp에서 역할을 조금 더 나눠볼 수 없을까요??

View를 나눠봐도 좋을 것 같고, 검색과 비디오를 관리하는 역할을 다른 모듈을 만들어 나눠주는것도 좋다고 생각해요.
함수단위로 분리를 해주시다보니 정작 중요한 책임을 YoutubeApp이 다 가지고 있어 비대한 느낌을 받았어요.
앱이 커져가면서 요구사항이 추가되면 어떤점이 문제가될지 생각해보고 개선방안을 생각해봅시다 :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

구조에 대한 설계 패턴은 이 포스팅을 훑어보시면 될 것 같아요.

해당 포스팅에서 프론트엔드가 거쳐온 아키텍처를 훑어볼 수 있어 좋아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

함수의 분리만 생각하고 정작 각자의 역할이 잘 나누어졌는지는 파악하지 못했네요...
기능 단위의 dom.js 대신 화면을 관리한다는 확실한 책임을 가진 View를 추가해보았습니다!

링크해주신 글도 너무 재밌게 잘 읽었어요 😆

@usageness
Copy link
Author

남겨주신 리뷰 모두 확인했고, 수정사항 반영하였습니다!

  • 테스트를 위한 테스트가 아닌, 꼭 필요한 테스트인지 확인하고 작성하기
  • 각 책임을 분리하여 한 곳에서 너무 많은 역할을 맡지 않도록 하기

미션의 규모가 작아 문제점이 분명하지 않은 현재로선 구조에 대해선 정해진 정답이 없는것 같습니다!
이에 대한 고민은 아직 혼자서 조금 더 해보는게 좋을 것 같네요 😂
이번에도 꼼꼼한 리뷰 정말 감사합니다!!!

Copy link

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 세지님 :)

피드백해주신 부분 확인했고 추가로 몇가지 코멘트를 더 적어봤어요.

미션의 규모가 작아 문제점이 분명하지 않은 현재로선 구조에 대해선 정해진 정답이 없는것 같습니다!

말씀하신대로 문제점이 보이지 않을때는 우선 작성하고 규모가 커졌다고 느껴지실때 분리해주셔도 좋아요.
나중에 구조를 분리하시게 되었을 때 분리하신 이유나 고민하셨던 점도 공유해주시면 좋겠어요 😄

한번 더 확인부탁드릴게요!

@@ -0,0 +1,71 @@
import { ERROR_MESSAGE } from "../../src/js/constants/constants";

describe("보고싶은 영상 찾기 모달창 전체 로직 테스트", () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cypress도 custom command를 이용해 추상화를 해보는것도 좋겠네요 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추상화를 진행하니 코드가 더 깔끔해보이네요! 적용했습니다 😄


it("보고싶은 영상 찾기 모달창 안에서 검색 결과 영역 스크롤 바를 끝까지 내리면 새로운 영상 10개를 추가로 사용자에게 보여준다.", () => {
cy.get(".video-list").scrollTo("bottom");
cy.get(".video-list").children().should("have.length", 20);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cy.get(".video-list").children().should("have.length", 20);
cy.get(".video-list").children().should("have.length", ITEMS_PER_REQUEST * 2);

설정에서 정해둔 상수를 사용해볼까요??


export default class VideoStorage {
constructor() {
this.storage = JSON.parse(localStorage.getItem("videos")) || [];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.storage = JSON.parse(localStorage.getItem("videos")) || [];
this.videos = JSON.parse(localStorage.getItem("videos")) || [];

videos가 더 어울린다고 생각되요 👀

generateTemplate.noResult()
);

insertImageSrc(document.querySelector(".no-result__image"), notFountImage);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insertImageSrc는 분리안해도 괜찮을 것 같은데 어떠실까요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리팩토링하면서 놓쳤던 부분이에요! 분리하지 않는 방향으로 수정했습니다!

</button>
</li>`;
},
videoItems(responseData, userStorage) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인자가 아직 userStorage에요 💦

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😭

return !inputValue.trim().length;
};

export const validateInput = (inputValue) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

input이 늘어났을 때 동일하게 검증을 한다면 그대로 두셔도 되겠지만 keyword만 검사하기 위한 함수만 고려하시고 작성하셨다면 checkKeywordValid는 어떨까요 👀

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

범용적으로 사용할 수 있을것 같은 함수라서 추상적으로 이름을 지었는데,
당장 input이 하나밖에 없는 상황이라 checkKeywordValid도 좋은 이름인것 같습니다!
수정했어요 😊

Comment on lines 2 to 22
const array = [];

for (let index = 0; index < 10; index++) {
array.push({
id: {
videoId: index,
},
snippet: {
channelTitle: "essential;",
thumbnails: {
high: {
url: "https://i.ytimg.com/vi/ECfuKi5-Cfs/hq720.jpg?sqp=-oaymwEcCOgCEMoBSFXyq4qpAw4IARUAAIhCGAFwAcABBg==&rs=AOn4CLDvmIcX-TgdcH2g_Bd4AUxw6hjmvQ",
},
},
publishTime: "2022-03-02T11:39:31Z",
title: "[Playlist] 너무 좋은데 괜찮으시겠어요?",
},
});
}

return array;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const array = [];
for (let index = 0; index < 10; index++) {
array.push({
id: {
videoId: index,
},
snippet: {
channelTitle: "essential;",
thumbnails: {
high: {
url: "https://i.ytimg.com/vi/ECfuKi5-Cfs/hq720.jpg?sqp=-oaymwEcCOgCEMoBSFXyq4qpAw4IARUAAIhCGAFwAcABBg==&rs=AOn4CLDvmIcX-TgdcH2g_Bd4AUxw6hjmvQ",
},
},
publishTime: "2022-03-02T11:39:31Z",
title: "[Playlist] 너무 좋은데 괜찮으시겠어요?",
},
});
}
return array;
return [...Array(10).keys()].map((index) => ({
id: {
videoId: index,
},
snippet: {
channelTitle: "essential;",
thumbnails: {
high: {
url: "https://i.ytimg.com/vi/ECfuKi5-Cfs/hq720.jpg?sqp=-oaymwEcCOgCEMoBSFXyq4qpAw4IARUAAIhCGAFwAcABBg==&rs=AOn4CLDvmIcX-TgdcH2g_Bd4AUxw6hjmvQ",
},
},
publishTime: "2022-03-02T11:39:31Z",
title: "[Playlist] 너무 좋은데 괜찮으시겠어요?",
},
})
)

아주 조금.. 더 깔끔하게 할 수 있어요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮😮😮... 이렇게 응용할수도 있었네요...!!!

@usageness
Copy link
Author

usageness commented Mar 13, 2022

추가 코멘트도 적용했습니다!

그리고 고치다 보니 버그가 있어서... 마저 수정했네요 ㅎㅎ f0bec40
다음엔 mock 데이터 테스트를 좀 더 손봐서 제출하도록 하겠습니다 😁

Copy link

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 세지님 ㅎㅎ

피드백 모두 잘 반영해주셔서 감사합니다~
추가적으로 확인해볼만한 부분이 있다면 eslint에서 에러가 표기되는 부분들이 있어 필요한 rule이라면 개선해보고 불필요하다고 생각되는 rule이라면 세지님이 lint설정을 다시 손봐주시면 좋겠습니다~

또 추가로 코멘트를 남겨드린 부분은 다음 미션에서 적용해주시면 되겠습니다 ㅎㅎ
다음 미션에서 뵐게요! 세지님 고생하셨어요 👍

Comment on lines +65 to +75
e.preventDefault();

const searchInputKeyword = document.querySelector("#search-input-keyword");
try {
checkKeywordValid(searchInputKeyword.value);
} catch ({ message }) {
alert(message);
return;
}

this.search(searchInputKeyword.value);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

검색을 여러번되는 현상을 막아봐도 좋을 것 같아요.
API를 중복해서 요청하지 않게 submit 시 throttle이나 debounce를 적용해보는건 어떠실까요?

Comment on lines +9 to +16
const date = rawDate.split("T")[0];
const standard = ["년", "월", "일"];

return date
.split("-")
.map((item, index) => Number(item) + standard[index])
.join(" ")
.trim();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

date를 파싱해서 그대로 사용해주셨는데 api에서 넘어오는 시간은 국제 표준시라 그대로 사용하게되면 일자가 안맞을 가능성이 있어보여요. date에 이해하는데 도움될만한 아티클이 있어 읽어보시길 추천드릴게요 😄

@EungyuCho EungyuCho merged commit fd9ea0d into woowacourse:usageness Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants