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

전역상태 zustand store로 리펙토링 #736

Merged
merged 8 commits into from
Oct 9, 2024

Conversation

Sang-minKIM
Copy link
Contributor

@Sang-minKIM Sang-minKIM commented Sep 28, 2024

Related issue

#673

Result

context를 store로 바꾸는 작업입니다.
작업을 하다보니 상태 하나씩 작업해서 PR을 올릴 수 없을 것 같아, 부득이하게 하나의 PR에 많은 코드 변경이 있을 것 같습니다....
커밋별로 보시고 리뷰해주시면 정말 감사하겠습니다🥹🥹

Work list

  • filteredRange context
  • branch 관련 context
  • repo context
  • owner context
  • data 관련 context

Discussion

  1. type 수정하면서 BranchListPayload의 위치가 고민입니다.
    현재는 store쪽에 선언하고 IDESentEvents에서 import 해서 쓰도록 했는데 이렇게 해도 괜찮을지 의견주시면 감사하겠습니다~~

  2. Dispatch<SetStateAction>
    기존에는 set 함수에 위의 타입을 사용했는데 zustand의 액션함수에 해당 타입을 사용하려면 아래와 같은 로직이 필요합니다.

type RepoStore = {
  repo: string;
  setRepo: Dispatch<SetStateAction<string>>;
};

export const useRepoStore = create<RepoStore>((set) => ({
  repo: "",
  setRepo: (repo) => set((state) => ({ repo: typeof repo === 'function' ? repo(state.repo) : repo })),
}));

위의 방식으로 Dispatch<SetStateAction>을 쓰는게 아래와 같은 방법으로 함수 타입을 설정하는 것보다 좋은점이 있는지 궁금합니다.

type RepoStore = {
  repo: string;
  setRepo: (repo: string) => void;
};

export const useRepoStore = create<RepoStore>((set) => ({
  repo: '',
  setRepo: (repo) => set({ repo }),
}));

@Sang-minKIM Sang-minKIM added this to the v0.7.2 milestone Sep 28, 2024
@Sang-minKIM Sang-minKIM self-assigned this Sep 28, 2024
@Sang-minKIM Sang-minKIM requested review from a team as code owners September 28, 2024 09:45
@Sang-minKIM Sang-minKIM marked this pull request as draft September 28, 2024 14:17
@Sang-minKIM Sang-minKIM modified the milestones: v0.7.2, v0.7.3 Sep 30, 2024
@Sang-minKIM Sang-minKIM changed the title filteredRange 리팩토링 전역상태 zustand store로 리펙토링 Sep 30, 2024
@Sang-minKIM Sang-minKIM marked this pull request as ready for review October 3, 2024 06:11
@Sang-minKIM
Copy link
Contributor Author

AS-IS

image

TO-BE

image

Context API를 Zustand Store로 바꾸는 작업 완료했습니다.
Summary의 항목을 클릭해서 커밋 내용 상세보기를 했을 때를 비교해보니 리렌더링이 줄어든걸 확실히 볼 수 있네요!
#684 에서 useShallow와 선택적 구독에 대한 내용 알려주신 @taboowiths 님 정말 감사합니다!

type 수정하면서 BranchListPayload의 위치 고민됨
현재는 store쪽에 선언하고 IDESentEvents에서 가져와서 쓰는데 괜찮은지 확인 필요
- 하나의 상태만 가져오는 경우 선택적 구독 방식 사용
- 여러 상태와 액션 함수를 가져와야하는경우 useShallow를 사용
- useShallow사용시 배열 방식을 이용해서 코드를 간결하게 작성
xxxjinn
xxxjinn previously approved these changes Oct 3, 2024
Copy link
Contributor

@xxxjinn xxxjinn left a comment

Choose a reason for hiding this comment

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

LGGGGTM 입니다!!!!!👍👍🥰🥰🥰

pcwadarong
pcwadarong previously approved these changes Oct 3, 2024
shgusgh12
shgusgh12 previously approved these changes Oct 3, 2024
Copy link
Contributor

@shgusgh12 shgusgh12 left a comment

Choose a reason for hiding this comment

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

LGTM입니다!!!!👍👍👍👍

@pcwadarong
Copy link
Member

LGTM입니다!!!!👍👍👍👍❗❗❗❗

joonwonBaek
joonwonBaek previously approved these changes Oct 4, 2024
Copy link
Contributor

@joonwonBaek joonwonBaek left a comment

Choose a reason for hiding this comment

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

LGTM 입니다!!

Copy link
Contributor

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

정말 고생많으셨습니다!! 💯 💯💯💯💯💯

자잘한 커멘트들 남겨놓았고,
전반적인 틀을 바꿔버린 만큼,
추후 테스트 (매뉴얼하게라도)도 좀 여러 방면으로 다 해보면 좋을 것 같습니다!!

(UI Test case들이 많이 올라오면 좋겠네요!!)

@Sang-minKIM Sang-minKIM merged commit 615f458 into githru:main Oct 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants