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.

Sign up for GitHub

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

Jump to bottom

Complete the integration of TypeScript with Redux #72

Merged
merged 5 commits into from
May 27, 2024
Merged

Complete the integration of TypeScript with Redux #72

merged 5 commits into from
May 27, 2024

Conversation

yumincho
Copy link
Member

@yumincho yumincho commented May 18, 2024 •
edited
Loading

Description

Redux TypeScript 도입을 마무리합니다.

Tasks

  • reducer에 return type 명시
  • @/redux 폴더 하위로 actions, reducers 폴더 이동
  • reducer들을 combine한 rootReducer 분리
// TypeScript 컴포넌트에서 RootState를 import해서 사용
export const SomeComponent = () => {
  const { user } = useSelector((state: RootState) => state.common.user);
  • App.jsx에 TypeScript 도입 (+ index.tsx 정리)

Notes

  • @/reducers/planner/itemFocus.ts에 explicit type casting을 추가하였습니다.
  case CLEAR_ITEM_FOCUS: {
    return {
      ...
    } as NoneItem; // use `as` keyword
  }

@yumincho yumincho added this to the TypeScript Migration milestone May 18, 2024
@yumincho yumincho self-assigned this May 18, 2024
@yumincho yumincho added the migrate migrate from JS CC to TS FC label May 18, 2024
@codecov Codecov
Copy link

codecov bot commented May 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 137 lines in your changes are missing coverage. Please review.

Project coverage is 4.46%. Comparing base (ccd274c) to head (54b4f9c).

Files Patch % Lines
src/App.tsx 0.00% 63 Missing ⚠️
src/index.tsx 0.00% 38 Missing and 1 partial ⚠️
src/redux/index.ts 0.00% 8 Missing ⚠️
src/redux/actions/common/index.ts 0.00% 4 Missing ⚠️
src/const.ts 0.00% 3 Missing ⚠️
src/redux/reducers/planner/itemFocus.ts 0.00% 1 Missing and 1 partial ⚠️
src/redux/reducers/timetable/semester.ts 0.00% 1 Missing and 1 partial ⚠️
src/redux/reducers/timetable/timetable.ts 0.00% 1 Missing and 1 partial ⚠️
src/redux/reducers/write-reviews/likedReviews.ts 0.00% 1 Missing and 1 partial ⚠️
src/redux/actions/common/user.ts 0.00% 1 Missing ⚠️
... and 11 more
Additional details and impacted files 
@@             Coverage Diff              @@
##           migration     #72      +/-   ##
============================================
- Coverage       4.47%   4.46%   -0.01%     
============================================
  Files            211     213       +2     
  Lines           6547    6558      +11     
  Branches        1640    1748     +108     
============================================
  Hits             293     293              
+ Misses          6158    6104      -54     
- Partials          96     161      +65

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yumincho yumincho changed the title Set up redux root Complete the integration of TypeScript with Redux May 19, 2024
@yumincho yumincho marked this pull request as ready for review May 19, 2024 12:27
@yumincho yumincho requested review from sboh1214 and useruseruse May 19, 2024 12:27
sboh1214
sboh1214 requested changes May 23, 2024
View reviewed changes
Copy link
Contributor

@sboh1214 sboh1214 left a comment

Choose a reason for hiding this comment

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

정말 많은 파일들에 변화가 있었는데 너무 수고 많으셨습니다!

src/shapes/state/planner/ItemFocus.ts 에서는 기존 타입에 문제가 있었는데, 리뷰 과정에서 잡아내지 못한 것이 아쉽네요 ㅠㅜ
src/App.tsx 나 src/index.tsx 는 다같이 리뷰하면 좋을 것 같습니다 :)

src/components/sections/account/FavoriteDepartmentsSubSection.jsx
@@ -8,7 +8,7 @@ import { appBoundClassNames as classNames } from '../../../common/boundClassName

import SearchFilter from '../../SearchFilter';

import { setUser } from '../../../actions/common/user';
import { setUser } from '../../../redux/actions/common/user';
Copy link
Contributor

Choose a reason for hiding this comment

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

저희 요거는 다 절대경로 임포트 사용 안하는건가요?

Copy link
Member Author

@yumincho yumincho May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

아 이번에 redux 디렉토리 하나 만들면서 여기저기 file change가 많은데, 제가 처음부터 끝까지 리팩토링 작업하지 않는 경우에는 일단 경로 수정하지 않고 있습니다 😂 change가 있어도 절대경로로 안 바뀐 파일들이 많을 거예요. 이번에는 두고 나중에 작업할 때 수정하면 될 것 같습니다. 예를 들어 FavoriteDepartmentsSubSection는 react query setup하는 브랜치에서 작업하면서 수정됐어요!

src/const.ts

export const TRACKING_ID = 'G-8NSY19J0T3';
export const STORAGE_KEY = 'otl-banner-key';
export const CAMPAIGN_KEY = 'CMPGN-2023-08-24-v1';
Copy link
Contributor

Choose a reason for hiding this comment

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

요거는 날짜가 들어가 있는 키인 것 같은데, 어떻게 작동하는 방식인지 설명 부탁드릴 수 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

@UrWrstNightmare 의 예전 배너 작업과 관련되어 있을 거예요. 로직은 자세히 안 보고 따로 const로 빼기만 해서 작동 방식은 잘 모릅니다 .. 🙄

sboh1214
sboh1214 approved these changes May 27, 2024
View reviewed changes
Copy link
Contributor

@sboh1214 sboh1214 left a comment

Choose a reason for hiding this comment

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

Simon Thumbs

@sboh1214 sboh1214 merged commit de39597 into migration May 27, 2024
3 of 5 checks passed
@sboh1214 sboh1214 deleted the migration@redux-root-setting branch May 27, 2024 15:38
@yumincho yumincho mentioned this pull request May 30, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sboh1214 sboh1214 sboh1214 approved these changes

@useruseruse useruseruse Awaiting requested review from useruseruse

Assignees

@yumincho yumincho

Labels
migrate migrate from JS CC to TS FC
Projects
None yet
Milestone
Refactoring & Migration
Development

Successfully merging this pull request may close these issues.
2 participants
@yumincho @sboh1214