implement & apply data fetching hook #49

sounmind · 2024-11-03T03:02:12Z

체크리스트

이슈가 연결되어 있나요?

참고해주셨으면 하는 사항

추가 변경: 기존 API 모듈에서 필요한 값만 반환하도록 변경했습니다.
3개 페이지에서 해당 훅을 호출하는 코드를 임시로 추가 했습니다.

DaleSeo

So far so good!

src/hooks/useMemberInfo.ts

src/hooks/useMemberInfo.test.ts

Sunjae95

member hook 에러처리까지 깔끔하게 구현하셨군요!
성공, 실패 두가지 케이스로 테스트 작성도 깔끔해서 코드읽기가 편했네요 👍
달레님 코멘트 있으니 approve는 달레님께 넘기도록 하겠습니다 :)

DaleSeo · 2024-11-06T03:07:15Z

달레님 코멘트 있으니 approve는 달레님께 넘기도록 하겠습니다 :)

@Sunjae95 승인은 양보하지 않으셔도 됩니다. 병합을 위한 최소 승인 개수는 하나이지만 둘 이상이 이상적인 것 같습니다 :)

src/hooks/useMembers.ts

DaleSeo · 2024-11-08T13:53:11Z

src/hooks/useMembers.ts

+function mapToMembers(members: MemberInfo[]): Member[] {
+  return members.map((member) => ({
+    id: member.id,
+    name: member.name,
+    cohort: member.cohort,
+    profileUrl: member.profileUrl,
+    progress: member.progress,
+    grade: member.grade,
+    solvedProblems: member.submissions.map(
+      (submission) => submission.problemTitle,
+    ),
+  }));
+}


api 모듈이 우리가 수정할 수 없는 써드 파티 모듈도 아니고 왜 이렇게 번거로운 데이터 맵핑을 해야하는지 모르겠습니다. MemberInfo 타입과 Member 타입을 구분해서 얻고자 하는 이득이 뭐죠? 과연 이렇게 소규모 프로젝트에서 동일한 엔티티를 가리키기 위한 타입을 2개를 유지할 필요가 있을까요? api 모듈의 MemberInfo 타입을 수정해서 (Info를 땠으면 좋겠습니다) hooks 모듈에서 그대로 사용하는 것이 더 이상적일 것 같은데 어떻게 생각하시는지요?

피드백 감사합니다!

현재 api 모듈(fetchLeaderborad)이 반환하는 값은 UI에서 필요하지 않는 값도 담겨 있는데요.
저의 의도는 api 모듈을 수정하지 않고도 독립적으로 hook이 UI에서 필요한 정보만 반환하게 만들고 싶었습니다. 말씀하신 대로 api 모듈을 외부 라이브러리처럼 바라봤어요.

api 모듈을 건드리지 않고 개발하고 싶은 마음이 컸던 것 같네요. 내심 api 모듈의 주인이 제가 아니라고 생각해 수정하기 꺼렸던 것 같습니다. 어쩌면 이 또한 소통 부재!

달레님 말마따나 상대적으로 작은 프로젝트에서 불필요한 행동일 수도 있겠습니다. api 모듈의 타입을 수정하고 그 타입을 hook에서 그대로 사용할 수 있도록 해볼게요!

이상적이긴 한데 코드 변경에 엄청난 부담이 가는 군요.. 일단 진행 중입니다. 오늘까지 변경사항 올려보겠습니다🙇‍♂️

- Ensure that only values that meet requirements are returned

sounmind · 2024-11-14T02:42:49Z

추가 변경: 기존 API 모듈에서 필요한 값만 반환하도록 변경했습니다.
- 여러 타입 이름과 함수 이름이 바뀌었습니다.
3개 페이지에서 해당 훅을 호출하는 코드를 임시로 추가 했습니다.

sounmind · 2024-11-14T02:51:37Z

src/api/services/common/types.ts

 export type Cohort = number;

-export type Member = {
+export type MemberIdentity = {


좋은 이름 추천 받습니다...

MemberInfo 혹은 MemberProfile.추천합니다..!

�Github Member Response에 기수를 추가하는 로직을 거치기에 필요한 타입이군요.
CoreMember는 어떨까요?
그러면 Member타입을 확장을 해줄수 있어서 클라이언트에서 상속받아 사용하기 좋아보인다고 생각해요.

src/api/services/store/storeService.ts

src/hooks/useMembers.test.ts

SamTheKorean · 2024-11-14T03:54:36Z

src/api/services/common/types.ts

 export type Cohort = number;

-export type Member = {
+export type MemberIdentity = {


MemberInfo 혹은 MemberProfile.추천합니다..!

src/api/services/process/processService.test.ts

SamTheKorean · 2024-11-14T04:08:28Z

전체적으로 리팩토링을 하시면서 불필요한 의존성과 타입 및 변수 사용을 줄이고 논리 구조를 단순화하려는 노력이 돋보인 것 같습니다! 기존 코드와 비교해가면서 검토했는데 확실히 가독성이 증가했고 달레님이 주신 피드백처럼 복잡한 mapping도 안하게 돼서 좋은 것 같습니다!

src/api/services/store/storeService.ts

src/api/services/common/types.ts

Sunjae95 · 2024-11-14T07:40:14Z

src/api/services/common/types.ts

 export type Cohort = number;

-export type Member = {
+export type MemberIdentity = {


�Github Member Response에 기수를 추가하는 로직을 거치기에 필요한 타입이군요.
CoreMember는 어떨까요?
그러면 Member타입을 확장을 해줄수 있어서 클라이언트에서 상속받아 사용하기 좋아보인다고 생각해요.

src/api/services/common/types.ts

sounmind · 2024-11-14T12:35:09Z

@DaleStudy/leaderboard PR이 계류된지 오래되기도 했고 다른 분들 개발에 병목이 되기도 하는 것 같아서, 심각한 이슈가 없는 한 오늘 중으로 병합하겠습니다.
그런 이슈가 있다면 코멘트 남겨주세요! 반영하겠습니다☺️

Sunjae95

수고하셨습니다!

sounmind linked an issue Nov 3, 2024 that may be closed by this pull request

Data-fetching hook #48

Closed

2 tasks

github-actions bot assigned sounmind Nov 3, 2024

sounmind marked this pull request as ready for review November 3, 2024 03:03

sounmind requested a review from a team as a code owner November 3, 2024 03:03

sounmind force-pushed the 48-data-fetching-hook branch from a61f510 to e5cb605 Compare November 3, 2024 03:03

DaleSeo reviewed Nov 5, 2024

View reviewed changes

src/hooks/useMemberInfo.ts Outdated Show resolved Hide resolved

src/hooks/useMemberInfo.ts Outdated Show resolved Hide resolved

src/hooks/useMemberInfo.test.ts Outdated Show resolved Hide resolved

src/hooks/useMemberInfo.test.ts Outdated Show resolved Hide resolved

Sunjae95 reviewed Nov 5, 2024

View reviewed changes

sounmind force-pushed the 48-data-fetching-hook branch 2 times, most recently from 6cfc282 to e3f9c86 Compare November 7, 2024 03:44

sounmind requested review from Sunjae95 and DaleSeo November 7, 2024 03:44

sounmind changed the title ~~feat: implement useMemberInfo~~ implement & apply useMemberInfo Nov 7, 2024

sounmind changed the title ~~implement & apply useMemberInfo~~ implement & apply useMembers Nov 7, 2024

sounmind changed the title ~~implement & apply useMembers~~ implement & apply data fetching hook Nov 7, 2024

sounmind force-pushed the 48-data-fetching-hook branch from e3f9c86 to e0479e7 Compare November 7, 2024 04:24

sounmind marked this pull request as draft November 8, 2024 01:41

sounmind force-pushed the 48-data-fetching-hook branch from e0479e7 to bf4e7f9 Compare November 8, 2024 02:27

sounmind marked this pull request as ready for review November 8, 2024 02:27

sounmind changed the title ~~implement & apply data fetching hook~~ implement data fetching hook Nov 8, 2024

DaleSeo reviewed Nov 8, 2024

View reviewed changes

feat: implement useMembers hook

80c1560

sounmind force-pushed the 48-data-fetching-hook branch from bf4e7f9 to 962a791 Compare November 14, 2024 02:37

sounmind requested a review from DaleSeo November 14, 2024 02:38

sounmind added 2 commits November 13, 2024 21:39

refactor: api module types

d0de9b7

- Ensure that only values that meet requirements are returned

feat: add sample hook call in pages

c8141ed

sounmind force-pushed the 48-data-fetching-hook branch from 962a791 to c8141ed Compare November 14, 2024 02:39

sounmind requested a review from a team November 14, 2024 02:48

sounmind commented Nov 14, 2024

View reviewed changes

sounmind force-pushed the 48-data-fetching-hook branch from bad8cc1 to b839cfb Compare November 14, 2024 03:07

sounmind changed the title ~~implement data fetching hook~~ implement & apply data fetching hook Nov 14, 2024

fix: test titles

48d414a

sounmind force-pushed the 48-data-fetching-hook branch from b839cfb to 48d414a Compare November 14, 2024 03:33

sounmind commented Nov 14, 2024

View reviewed changes

src/api/services/store/storeService.ts Show resolved Hide resolved

SamTheKorean approved these changes Nov 14, 2024

View reviewed changes

Sunjae95 reviewed Nov 14, 2024

View reviewed changes

refactor: convert Grade to enum

f124cf8

sounmind requested a review from Sunjae95 November 14, 2024 12:35

Sunjae95 approved these changes Nov 14, 2024

View reviewed changes

sounmind merged commit 4ada7ce into main Nov 14, 2024
1 check passed

sounmind deleted the 48-data-fetching-hook branch November 14, 2024 19:38

DaleSeo mentioned this pull request Nov 23, 2024

배포 환경에 빠진 환경 변수 설정 #82

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement & apply data fetching hook #49

implement & apply data fetching hook #49

sounmind commented Nov 3, 2024 •

edited

Loading

DaleSeo left a comment

Sunjae95 left a comment

DaleSeo commented Nov 6, 2024

DaleSeo Nov 8, 2024

sounmind Nov 8, 2024

sounmind Nov 14, 2024

sounmind commented Nov 14, 2024

sounmind Nov 14, 2024

SamTheKorean Nov 14, 2024

Sunjae95 Nov 14, 2024

SamTheKorean Nov 14, 2024

SamTheKorean commented Nov 14, 2024

Sunjae95 Nov 14, 2024

sounmind commented Nov 14, 2024

Sunjae95 left a comment

implement & apply data fetching hook #49

implement & apply data fetching hook #49

Conversation

sounmind commented Nov 3, 2024 • edited Loading

체크리스트

참고해주셨으면 하는 사항

DaleSeo left a comment

Choose a reason for hiding this comment

Sunjae95 left a comment

Choose a reason for hiding this comment

DaleSeo commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sounmind commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamTheKorean commented Nov 14, 2024

Choose a reason for hiding this comment

sounmind commented Nov 14, 2024

Sunjae95 left a comment

Choose a reason for hiding this comment

sounmind commented Nov 3, 2024 •

edited

Loading