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

[2팀 이수현] [Chater 1-3] React, Beyond the Basics #55

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

lshyun955
Copy link

@lshyun955 lshyun955 commented Jan 2, 2025

과제 체크포인트

기본과제

  • shallowEquals 구현 완료
  • deepEquals 구현 완료
  • memo 구현 완료
  • deepMemo 구현 완료
  • useRef 구현 완료
  • useMemo 구현 완료
  • useDeepMemo 구현 완료
  • useCallback 구현 완료

심화 과제

  • 기본과제에서 작성한 hook을 이용하여 렌더링 최적화를 진행하였다.
  • Context 코드를 개선하여 렌더링을 최소화하였다.

과제 셀프회고

기술적 성장

  • react를 잘 알지 못해서 공식문서를 1회독하고 기본과제를 완료하였고, 심화과제도 공식문서를 참조하여 완료하였습니다!
  • useMemo, useCallback, memo를 대략적으로 어떤 상황에서 사용해야하는지 알게 되었고, 간단한 코드를 읽어보며 이곳에서 해당 훅이 사용되는게 맞는지 의문을 가질 수 있는 수준까지는 성장한 것으로 생각합니다!

발제자료와 더불어 공식문서를 정독하니 확실히 좋네요.. 다시금 공식문서의 중요성을 알게됩니다..(과제에대한 내용도 공식문서만 보면 다 구현할 수 있더라구요..)

코드 품질

context를 인증 관련 auth, 아이템 관련 item, 테마 관련 theme, 알림 관련 �app으로 구분하여 개인적으로는 다시 코드를 볼 때도 이전 과제들 보다 편했습니다!

학습 효과 분석

공식문서 최고...
공식문서 읽어보면서 문서 내 참조걸린 문서까지 훑어보니까 정말 개념 이해하는데 최고였습니다..
리액트 공식문서 3회독 실시..

과제 피드백

이번 과제를 통해서 리액트 hook에 대해 자세히 알게 되었고, hook을 이해하고 구현하고 최적화까지 하는 과제가 만족스러웠습니다!

리뷰 받고 싶은 내용

  • provider를 따로 파일로 분리하지 않고 context파일 내에서 export하고 있는데, 파일을 분리하는게 더 올바른 방향일까요?
    -> React Fast Refresh 문제가 발생해서 확인해보니 https://github.com/vitejs/vite-plugin-react-swc#consistent-components-exports 에서 React 새로고침이 제대로 작동하려면 파일이 React 컴포넌트만 내보내야 한다고 적혀있어서 파일 분리 하였습니다 ㅎ.ㅎ
  • 테마 상태값에 따른 다크모드 버튼명 변경처리도 Header컴포넌트에 존재하여, 헤더 부분의 다크모드 클래스 변경 처리를 Header 컴포넌트로 옮겼는데 이렇게 header컴포넌트에서 테마 상태에 따라 ui가 변경되는 로직을 다 넣는게 올바른 방법인지 궁금합니다!

import { AppContextType } from "./types";
import { AppContext } from "./AppContext";

export function AppProvider({ children }: { children: React.ReactNode }) {

Choose a reason for hiding this comment

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

다른 컨텍스트를 관리하는 Provider를 보면 각 역할에 맞는 네이밍의 Provider로 되어있는 것 같은데 여기만 특별히 AppProvider라는 네이밍이 된 이유가 있을까요?!

수현님의 생각이 궁금합니다 !

Copy link
Author

Choose a reason for hiding this comment

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

맞아요! 저도 코드를 다 작성하고나서 보니까 AppProvider, AppContext라는 네이밍이 너무 러프하게 작성되어 있다고 생각했어요!...ㅠ
그래서 다른 분들 코드를 참고했는데, 대부분 Notification~와 같이 작성하셨더라구요...
저의 무의식이 저렇게 네이밍을 하게 했습니다...ㅋㅋ ㅠ

Comment on lines +6 to +21
export const ItemProvider = ({ children }: { children: React.ReactNode }) => {
const [items, setItems] = useState(generateItems(1000));

const addItems = () => {
setItems((prevItems) => [
...prevItems,
...generateItems(1000, prevItems.length),
]);
};

const value: ItemContextType = {
items,
addItems,
};
return <ItemContext.Provider value={value}>{children}</ItemContext.Provider>;
};
Copy link

@Songchangyeop Songchangyeop Jan 3, 2025

Choose a reason for hiding this comment

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

수현님 ItemProvider에서 쓰이는 로직들은 ItemList에서만 사용되기 때문에 따로 Context로 처리하지 되지 않을까 생각이 듭니다 !

제가 생각한 이유는 이렇습니다 !

  1. items, addItems는 ItemList에서만 쓰이기 때문에 props drilling도 없어서 따로 외부에서 상태를 관리 하지 않아도 될 것 같아요
  2. 만약 items 상태가 변하게 된다면 ItemProvider가 감싸는 모든 하위 children이 리렌더링 되어서 불필요한 렌더링 낭비가 일어날 것 같습니다 ItemList 내부에서 사용하면 해소 될 것 같아요

Profiler를 통해 addItems 함수가 호출되었을 떄 렌더링이 어디서 일어나는지 한번 살펴보면 좋을 것 같습니다 👍

Copy link
Author

Choose a reason for hiding this comment

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

오 그렇네요! ItemProvider의 로직이 결국 ItemList에서만 쓰이는데, Header와 폼 컴포넌트까지 감싸고 있으니 불필요한 렌더링이 발생하고 있었어요! 말씀해주신대로 한 번 수정해보겠습니다! ㅎㅎㅎㅎ

Comment on lines +1 to +5
export * from "./Header";
export * from "./ComplexForm";
export * from "./ItemList";
export * from "./NotificationSystem";
export * from "./ItemListContainer";

Choose a reason for hiding this comment

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

배럴 파일을 만들어 주셨네요! 👍

저도 만들까 하다가 고민이 된 부분이
components 폴더 내부에 지금은 5개의 컴포넌트 뿐이지만 규모가 커지게 된다면 배럴파일 내부에 export 하는 컴포넌트가 엄청 많아질 것 같아서 안 만들게 되었는데요
그래서 저는 보통 components 내부에 같은 도메인 컴포넌트 끼리 묶어 폴더를 만들고 해당 폴더 내부에서만 배럴 파일을 만드는데 수현님은 어떻게 생각하시는지 궁금합니다

어떻게 배럴파일을 잘 관리해야할지 잘 모르겠네요 🤔

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.

음 저도 파일이 몇개 없어서 하나의 배럴파일로 모든 컴포넌트를 관리했는데요..
이렇게 되면 창엽님이 말씀해주신대로 컴포넌트가 많아지게 됐을 때 관리가 힘들 것 같긴하네요..
도메인 별로 주석을 구분할까? 생각도 했는데, 뭔가 도메인별로 디렉터리를 분리하고 각 디렉터리에서 배럴 파일을 만들고 상위 디렉터리에서 배럴 파일을 만드는 방식으로 가면 어떨까요??

@Songchangyeop
Copy link

수현님 이번 과제 고생 많으셨습니다 ! 백엔드 개발자 이신데도 항상 끝까지 과제 완수하시는 모습이 진짜 배울점이 많은 것 같습니다:)
새해 복 많이받으세요 !

@gmkim716
Copy link

gmkim716 commented Jan 3, 2025

과제하느라 수고 많으셨습니다. 창엽님께서 좋은 피드백을 많이 남겨주셔서 유의미한 코드리뷰는 없지만, 응원을 커밋으로 남깁니다. 이번주는 수현님처럼 공식문서를 통해 개념을 타파해가야겠다라는 목표를 세울 수 있었던 점이 과제 외에 제게 큰 피드백이 되었네요. 같이 공식문서 읽으러 입소하겠습니다. 악

@lshyun955
Copy link
Author

수현님 이번 과제 고생 많으셨습니다 ! 백엔드 개발자 이신데도 항상 끝까지 과제 완수하시는 모습이 진짜 배울점이 많은 것 같습니다:) 새해 복 많이받으세요 !

항상 창엽님 코드를 보며 리뷰를 보며 많이 배워갑니다... 창엽님과 팀원분들 따라서 끝까지 완주하겠습니다...
창엽님도 새해에는 행복한 일만 가득하시길 바랄게요! ㅎ.ㅎ

@lshyun955
Copy link
Author

과제하느라 수고 많으셨습니다. 창엽님께서 좋은 피드백을 많이 남겨주셔서 유의미한 코드리뷰는 없지만, 응원을 커밋으로 남깁니다. 이번주는 수현님처럼 공식문서를 통해 개념을 타파해가야겠다라는 목표를 세울 수 있었던 점이 과제 외에 제게 큰 피드백이 되었네요. 같이 공식문서 읽으러 입소하겠습니다. 악

항상 열심히하시는 경민님을 보며 자극을 받아요ㅎ.ㅎ
남은 기간도 저와 팀원분들과 함께 으쌰으쌰해서 잘 마무리하면 좋겠습니다 ㅎㅎㅎ
같이 공식문서 스터디 시작해볼까요?ㅋㅋㅋㅋㅋ

새해 복 많이받으세영~~~~~ ㅎㅎㅎㅎ

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.

3 participants