Skip to content
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단계 - 영화 목록 불러오기] 버건디(전태헌) 미션 제출합니다. #101

Merged
merged 71 commits into from Mar 25, 2024

Conversation

brgndyy
Copy link

@brgndyy brgndyy commented Mar 21, 2024

배포링크

안녕하세요 지그 처음 뵙겠습니다

버건디라고 합니다!

이번에 학습 목표였던 API 연동에 포커스를 맞추고 한번 작업해보았습니다.

- 파일 구조

📦src
 ┣ 📂api // 비동기 통신을 담은 클래스
 ┣ 📂components // 컴포넌트들
 ┣ 📂constants
 ┃ ┗ 📂api  // 비동기 통신을 위한 상수 옵션
 ┣ 📂error // 에러 관련 커스텀 클래스
 ┣ 📂mocks // 목데이터
 ┣ 📂services // 서비스 폴더
 ┣ 📂types => 타입 폴더
 ┣ 📂utils => 유틸 함수들을 담은 폴더
 ┗ index.js

기본적으로 컴포넌트들은

Component.js
render.ts
eventHandler.ts

의 구성을 띄우고 있는데요.

컴포넌트는 컴포넌트명, render는 단순 UI 작업, eventHandler는 이벤트 핸들러 관련 로직이 들어가 있습니다.

그 외에 여쭤보고싶은 부분에 대해서 댓글로 한번 남겨보도록 하겠습니다. 잘부탁드립니다 🙇

brgndyy and others added 30 commits March 19, 2024 14:38
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
brgndyy and others added 5 commits March 21, 2024 15:09
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: 0jenn0 <jenn0.6n@gmail.com>
Co-authored-by: river <0jenn0@users.noreply.github.com>
src/api/index.ts Show resolved Hide resolved
src/services/UIFeedBackManager.ts Outdated Show resolved Hide resolved
Array.from({ length: this.SKELETON_LENGTH }, () => {
const skeleton = Skeleton();
fragment.appendChild(skeleton);
});
Copy link
Author

Choose a reason for hiding this comment

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

지금 여기서 스켈레톤 컴포넌트를 만들어줄때, 어차피 데이터를 받아오기 전에 띄워주어야하는 UI이다보니 데이터 갯수를 정확히 판단하기 어렵다는 생각이 들어서, 임의로 바로 view에 보이는 8개의 스켈레톤을 만들어주어서 컴포넌트로 띄워주었는데요.

이 미션 전에 다른 프로젝트나 경험들에서는 하나의 스켈레톤 UI가 전체 컨텐츠를 대체하는 구조로 만들었었습니다. (로딩스피너가 전체 페이지를 차지하는 구조 )

근데 지금 이렇게 컨텐츠 갯수를 맞춰서 스켈레톤을 띄워주는건 처음이라서 이런식으로 구현한게 맞는 방법일까 ? 싶은 궁금증이 있습니다.

데이터를 받아오기전의 상태 UI여서, 데이터 갯수를 파악하는거 자체가 불가능하니 임의로 갯수에 맞는 스켈레톤 UI를 넣어준다. 라는 판단, 잘 짚은 부분일까요 ?

혹시 잘못 짚고 있는 부분이 있다면 말씀주시면 감사하겠습니다 🙇

Copy link

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.

실무에서는, 스크롤마다 서버에서 내려주는 데이터의 개수를 고정하여 미리 알 수 있도록 하는 경우가 많습니다.

라고 말씀해주셨는데요.

혹시 이 부분 같은 경우 어떻게 구현이 되는건지 여줘보고 싶습니다.

데이터의 개수를 고정하여 라는 것이, 백엔드와 서로 협의하에 데이터 갯수를 미리 정해놓고 프론트 측에서 임의로 갯수를 고정해놓고 스켈레톤으로 대체해준다는 뜻이실까요 ?

저는 이 부분을 리액트로 대체해서 생각해본다고 해도, 리액트 쿼리를 사용하든 다른 api를 사용하든간에 데이터를 받아야지만 해당 데이터의 갯수를 파악해서 스켈레톤을 띄워줄수 있지 않나? 라는 생각이 계속 들어서요..!

제가 잘못 이해하고 있는 부분이 있다면 말씀 주시면 감사하겠습니다 🙇

Copy link

@zigsong zigsong Mar 25, 2024

Choose a reason for hiding this comment

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

데이터의 개수를 고정하여 라는 것이, 백엔드와 서로 협의하에 데이터 갯수를 미리 정해놓고 프론트 측에서 임의로 갯수를 고정해놓고 스켈레톤으로 대체해준다는 뜻이실까요 ?

넵 맞습니다. 예를 들면, 서버 응답을

{
  count: 10, // (마지막 페이지를 제외하고) 항상 이 값으로 내려옴)
  data: [...] 
}

이렇게 내려주기로 약속한다면, 프론트에서는 스켈레톤을 10개로 고정하고 띄워주면 되겠죠.

이것은 리액트의 문제도, 리액트 쿼리의 문제도 아닌 단순 서버와 프론트의 '약속'일 뿐입니다.

Object.setPrototypeOf(this, new.target.prototype);
}
}

Copy link
Author

Choose a reason for hiding this comment

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

원래 UIFeedbackManager 클래스에서. error.status 상태 값에 따라서 그에 맞는 에러 컴포넌트를 띄워주려고 했는데요.

Error 타입 자체에 status 상태 값이 없다보니 HttpError라는 커스텀 에러 클래스를 만들어주어서 status 상태값을 넣어주려고 했는데

저렇게 단순히 this.status = status 만으로는 주입이 안되어서 검색 해서

 Object.setPrototypeOf(this, new.target.prototype);

를 사용해서 status 상태값을 넣어줄수 있었습니다.

근데 프론트 측에서 status 상태값을 넣어주는 방법이 이게 최선일까요..?

보통 저 같은 경우에 혼자 프로젝트를 진행했었을때 백엔드측에서

class HttpError extends Error {
  code: number;

  constructor(message: string, errorCode: number) {
    super(message);
    this.code = errorCode;
  }
}

export default HttpError;

이렇게 생성을 해주고

에러를 던져줄때

catch(error){
const error = new HttpError('에러가 발생했어요!', 503);
throw error;
}

이런 식으로 직접 상태 코드를 지정해서 프론트쪽으로 던져주는 방식으로 구현을 했었는데,

이번에는 받기만 해야하다보니까 이런식으로 상태코드를 지정하는게 맞는건지 ? 싶습니다..

에러 관련 status를 지정해주는 방식이 저 방식 밖에 없을까요 ?

제가 잘못 짚고 있는 부분 말씀주시면 감사할거 같아요!

Copy link

Choose a reason for hiding this comment

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

말씀주신 대로 프로젝트마다 서버에서 에러 응답코드를 어떻게 내려주느냐에 따라 프론트에서 처리해야 하는 방식이 달라집니다. 어느 정도는 서버에 달려있고, 그렇기에 협업 시에 커뮤니케이션이 필요하기도 하죠.

버건디가 지금 작성해주신 대로 HttpError 클래스는 Error를 상속해서 status를 추가해주는 것이 일반적인데, 어떤 문제 때문에 이 코드

Object.setPrototypeOf(this, new.target.prototype);

가 필요하셨나요? 버건디의 코드를 받아 실행해봐도, 해당 코드가 없어도 정상적으로 동작합니다.

문제를 조금 더 구체적으로 설명해주시면 같이 보겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

HttpError 클래스는 Error를 상속해서 status를 추가해주는 것이 일반적 이라고 말씀 주신 부분에 대해서 저도 공감하고 �예상한 동작인데요.

이번 미션에선 왜 계속 이 status 속성을 catch 절에서 못받는건지 이해가 안되더라구요🥲

일단 지그가 해당 코드 없이도 정상적으로 동작한다고 말씀주셨으니, 제가 잘못 짚은점이 있을거라고 생각 되네요.

저도 한번 다시 확인해보도록 하겠습니다! 직접 테스트 해주셔서 감사합니다 🙇

Copy link
Author

Choose a reason for hiding this comment

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

스크린샷 2024-03-22 오후 1 21 48 스크린샷 2024-03-22 오후 1 22 13

안녕하세요 지그 ! 지금 확인해보았을때

    Object.setPrototypeOf(this, new.target.prototype);

이 함수가 있으면 404 페이지가 렌더링 되고, 없으면 콘솔에만 에러메세지가 나오는 상황이에요!

Copy link

Choose a reason for hiding this comment

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

안녕하세요 버건디, 혹시 리뷰를 반영하는 과정에서 API_KEY를 받아오는 로직을 수정하셨나요?

이전에는 .env 파일 없이도 로컬에서 앱이 잘 실행되었던 것 같은데, 지금 .env 파일이 없어 401 에러가 발생하고 있습니다.

Copy link

Choose a reason for hiding this comment

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

.env 내용 전달 받아서 확인했습니다 :)

@brgndyy brgndyy changed the base branch from main to brgndyy March 21, 2024 07:54
src/types/element.d.ts Outdated Show resolved Hide resolved
Copy link

@zigsong zigsong left a comment

Choose a reason for hiding this comment

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

안녕하세요 버건디, 리뷰를 맡은 지그입니다.

앱을 구성하는 각 컴포넌트를 잘 구성하셨고, 기능/UI도 적절하게 분리하셔서 코드의 가독성이 매우 좋네요.

추가적으로 개선할 수 있는 부분 및 버건디의 질문에 대해 코멘트 남겼습니다 :)

src/components/Header/render.ts Show resolved Hide resolved
src/components/Header/Header.ts Outdated Show resolved Hide resolved
src/utils/isHTMLElement.ts Show resolved Hide resolved
src/utils/createElement.ts Show resolved Hide resolved
src/components/BasicFrame/BasicFrame.ts Show resolved Hide resolved
src/components/Header/render.ts Outdated Show resolved Hide resolved
src/components/MovieItem/render.ts Outdated Show resolved Hide resolved
Object.setPrototypeOf(this, new.target.prototype);
}
}

Copy link

Choose a reason for hiding this comment

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

말씀주신 대로 프로젝트마다 서버에서 에러 응답코드를 어떻게 내려주느냐에 따라 프론트에서 처리해야 하는 방식이 달라집니다. 어느 정도는 서버에 달려있고, 그렇기에 협업 시에 커뮤니케이션이 필요하기도 하죠.

버건디가 지금 작성해주신 대로 HttpError 클래스는 Error를 상속해서 status를 추가해주는 것이 일반적인데, 어떤 문제 때문에 이 코드

Object.setPrototypeOf(this, new.target.prototype);

가 필요하셨나요? 버건디의 코드를 받아 실행해봐도, 해당 코드가 없어도 정상적으로 동작합니다.

문제를 조금 더 구체적으로 설명해주시면 같이 보겠습니다.

src/services/UIFeedBackManager.ts Outdated Show resolved Hide resolved
Array.from({ length: this.SKELETON_LENGTH }, () => {
const skeleton = Skeleton();
fragment.appendChild(skeleton);
});
Copy link

Choose a reason for hiding this comment

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

아하... 데이터의 개수를 파악하기가 어려운 상태라면 임의 개수의 스켈레톤을 넣어주는 것이 최선같아 보이네요. 사용자가 추가 스크롤 없이 한 화면에서 볼 수 있는 아이템의 최대 개수라면 적절할 것 같습니다.

실무에서는, 스크롤마다 서버에서 내려주는 데이터의 개수를 고정하여 미리 알 수 있도록 하는 경우가 많습니다.

@zigsong
Copy link

zigsong commented Mar 21, 2024

참고로 제가 금주 3/22(금)~3/24(일) 부재로 리뷰가 어려울 것 같습니다. 리뷰 반영해주시면 월요일에 빠르게 보도록 할게요. 미리 양해 말씀 드립니다 🙇‍♀️

Copy link
Author

@brgndyy brgndyy left a comment

Choose a reason for hiding this comment

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

안녕하세요 지그 말씀하신 부분 수정해서 한번 올려봅니다! 좋은 주말 되세요 🙇

src/api/index.ts Show resolved Hide resolved
Copy link

@zigsong zigsong left a comment

Choose a reason for hiding this comment

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

안녕하세요 버건디! 우선 리뷰 반영 늦게 확인하여 죄송하다는 말씀 드립니다 🙇‍♀️

질문이나 의견 많이 남겨주셨는데요, 제가 아는 한에서 코멘트 남겼습니다.

HttpError와 관련해서 수정하고 싶으신 부분이 있으실 것 같아 request changes 드리는데요, 수정하시지 않아도 괜찮다고 생각하여 혹시 말씀주시면 바로 머지하겠습니다.

.gitignore Outdated Show resolved Hide resolved
Comment on lines 11 to 13
// function isHttpError(error: any): error is HttpError {
// return error instanceof HttpError && typeof error.status === 'number';
// }
Copy link

Choose a reason for hiding this comment

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

.env 파일의 값을 몰라 제가 직접 실행해보진 못했는데요,

이곳에서 확인하고자 하는 과정에 살짝 오류가 있었던 것 같습니다.

catch문에서 잡은 errornew HttpError() 등의 생성자로 생성된 것이 아니기 때문에 error instanceof HttpError가 당연히 false가 나올 수밖에 없구요, 그래서 이 함수를 쓰려면 HttpError 클래스 내에서 Object.setPrototypeOf(this, new.target.prototype);를 써야 했던 것으로 추측됩니다.

function isHttpError(error: any): error is HttpError {
  return typeof error.status === 'number';
}

이렇게만 수정해줘도 괜찮습니다.

Copy link
Author

Choose a reason for hiding this comment

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

헉 맞네요.. 제가 놓쳤습니다.

직접 수정해주셔서 감사합니다 !

Copy link

@zigsong zigsong left a comment

Choose a reason for hiding this comment

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

안녕하세요 버건디! 코멘트가 많았는데, 버건디만의 스타일로 모두 잘 반영해주신 것 같네요 👍

미션을 통해 API 호출과 비동기 처리, 에러핸들링까지 많이 고민하고 개발하신 흔적이 느껴집니다. 그만큼 많이 성장하셨을 것이라 생각되네요 😉

2단계 미션에서 또 만나요. 화이팅 🚀

@zigsong zigsong merged commit 1b53ba5 into woowacourse:brgndyy Mar 25, 2024
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.

None yet

2 participants