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

Configure TypeScript (DL-17) #53

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Configure TypeScript (DL-17) #53

wants to merge 2 commits into from

Conversation

r0o0
Copy link
Collaborator

@r0o0 r0o0 commented Jun 14, 2020

  • Configure TypeScript

타입스크립트 적용을 했는데 redux createAsyncThunk helper 함수 만드는 작업이 남았어요.

@r0o0 r0o0 added the front-end front-end pull request label Jun 14, 2020
@r0o0 r0o0 requested review from syouschoi, jeux1204, codemacaron and a user June 14, 2020 04:02
@r0o0 r0o0 self-assigned this Jun 14, 2020
Copy link
Collaborator

@recordar recordar left a comment

Choose a reason for hiding this comment

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

ReduxSample 은 잘 동작하는거죵? :))
몇가지 코멘트가 있긴한데,
고생많으셨습니다!!

theme?: 'primary' | 'secondary';
};

export type Props = AttributeProps & CustomProps;
Copy link
Collaborator

Choose a reason for hiding this comment

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

interface 대신 type 키워드로 컴포넌트 prop 스펙을 정의해도 상관없긴 한데,
확장성 같은 몇가지 기능 차이가 있어욤 ㅎㅎ

차후 다른 형태의 버튼 인터페이스가 필요하다면
기존 인터페이스를 extends 해서 확장 가능해요 type 은 안되구요ㅠㅠ

그래서 아래와 같이 쓰는건 어떨까요 ? :))

export enum THEME_VALUE {
  PRIMARY ='primary',
  SECONDARY= 'secondary'
}

export interface IButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> {
  theme?: THEME_VALUE
}

Copy link
Collaborator Author

@r0o0 r0o0 Jun 26, 2020

Choose a reason for hiding this comment

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

아래 문서 보고 정한것인데 컴포넌트 내에서 사용하는 Props나 State는 type으로 정의하고 API 관련된 타입들은 interface를 사용하고자 합니다. 타입스크립트에서도 권장하는 ruleset에서 interface-over-type-literal을 제거 했더라구요.

@@ -0,0 +1,7 @@
export interface UserSampleState {
Copy link
Collaborator

Choose a reason for hiding this comment

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

인터페이스 정의할 때는 앞에 I 붙여 주는게 좋아욤 ㅎㅎ
export interface IUserSampleState { ...

tslint 로 자동으로 체크해 줄 수도 있구요
https://palantir.github.io/tslint/rules/interface-name/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이 부분은 naming convention이라 정하기 나름일것 같은데 I prefix로 interface라는걸 명시해 주는 이유가 있을까요? I prefix가 모든 interface에 들어간다면 I 로 시작하는 interface 명이나 interface 명이 앞에있는 I로 인해 잘 안보일까 걱정이네요

export interface UserSampleState {
id: number;
email: string;
first_name: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

필드이름은 camel case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이부분 제가 잘 몰라서 그러는데 데이터가 first_name으로 들어와도 camelCase로 쓰는건가요?

export interface UserSampleState {
id: number;
email: string;
first_name: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 camel case :))

@@ -0,0 +1,16 @@
export interface UserSampleState {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IUserSampleState :))

import { createAction } from '@reduxjs/toolkit';

function actionWithPayload<T>() {
return (payload?: T) => ({ payload });
Copy link
Collaborator

Choose a reason for hiding this comment

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

payload 가 옵셔널이라, undefined 인 경우 처리도 필요할것 같아욤 !!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

function actionWithPayload<T>() {
  return (payload?: T): { payload: T | undefined } => ({ payload });
}

이렇게 처리했는데 말씀하시는게 맞는지 모르겠네용 ㅎㅎ

[ReduxSampleActions.getSampleUsers.fulfilled]: (state, { payload: userData }) => {
console.log(userData);
Copy link
Collaborator

Choose a reason for hiding this comment

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

console.log 빼주세욤 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 넵ㅎㅎ

const reduxSampleReducer = createReducer(initialState, {
// TODO createAsyncAction helper 생성 후 fulfilled 상태 타입 확인
[ReduxSampleActions.getSampleUsers.fulfilled]: (state, { payload: userData }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[ReduxSampleActions.getSampleUsers.fulfilled.type]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아하 type을 넣으면 됐었네요! ㅎㅎ

state.sampleUsers = userData;
},
[ReduxSampleActions.triggerBtnClick]: (state, action) => {
[TRIGGER_BTN_CLICK]: (state, action) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[ReduxSampleActions.triggerBtnClick.type]:
...
[ReduxSampleActions.reset.type]: 

이렇게 써도 될 것 같아욤ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵~!


export default function configureAppStore(preloadedState) {
export type AppDispatch = ReturnType<typeof configureAppStore>['dispatch'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

export type TAppDispatch = ... 처럼 type 은 T prefix~

https://luckyyowu.tistory.com/401

@r0o0
Copy link
Collaborator Author

r0o0 commented Jun 26, 2020

ReduxSample 은 잘 동작하는거죵? :))
몇가지 코멘트가 있긴한데,
고생많으셨습니다!!

개발 서버 돌려야된다는걸 잊고 그냥 올렸네요 ㅠㅠ 고칠거 고치고 이제 개발 서버도 ReduxSample도 잘 동작합니다~ ㅎㅎㅎ :)
궁금한 점들이랑 naming convention에 관해 질문 드렸어요 바쁘실텐데 코멘트 남겨주서서 감사합니다~!! 그리고 다시 한번 잘부탁드립니당~~

@r0o0 r0o0 requested a review from recordar June 26, 2020 15:40
@r0o0
Copy link
Collaborator Author

r0o0 commented Jun 26, 2020

AppState 및 스토어 쪽에 circular dependency가 생긴것들 store쪽에 구조 조정을 조금 했는데 src/store/actions.ts 파일에서도 발생하는데 이건 아직 해결을 못했어요... ㅜㅜ 지금은 방법이 생각이 안나서 나중에 다시 볼게요

@r0o0
Copy link
Collaborator Author

r0o0 commented Jun 28, 2020

@recordar 피드백 해주신거 적용하고 개발 서버랑 styleguide 서버 원활히 작동하는것 같아서 일단 #60 PR에 이 PR과 같은 코드를 올려 머지 하고 작업 진행하고 있을게요~ 시간 되실 때 천천히 봐주세요~!! 부탁드립니다~ :)

이 PR은 추가 수정을 위해 남겨 놓겠습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end front-end pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants