-
Notifications
You must be signed in to change notification settings - Fork 2
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
[이예진]TS Todo Refactor 과제 #4
base: yejin
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new 연산자를 사용하지 않고 화살표 함수로 구현하셨네요?!
class 형태가 아니라 이렇게 변환해도 되는 거였군요?!
깔끔한 함수화는 역시 예진님 트레이트마크네요 배워갑니다 총총 :)
todoDiv.className = 'list_div'; | ||
|
||
// prettier-ignore | ||
const todoLi = createTodoElement({$target: todoDiv,element: "li",idx,text}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const todoLi = createTodoElement({$target: todoDiv,element: "li",idx,text}); | |
const todoLi: HTMLLIElement= createTodoElement({$target: todoDiv,element: "li",idx,text}); |
P5: todoLi
가 any 타입이네요! 생성하면서 HTMLLIElement
을 지정해주면 좋을 거 같습니다.
}); | ||
|
||
// prettier-ignore | ||
const newTodoBtn = createTodoElement({$target: todoDiv, element:"button", idx, text:"🗑️"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const newTodoBtn = createTodoElement({$target: todoDiv, element:"button", idx, text:"🗑️"}) | |
const newTodoBtn: HTMLButtonElement = createTodoElement({$target: todoDiv, element:"button", idx, text:"🗑️"}) |
P5: 위와 마찬가지로 HTMLButtonElement
으로 명시해주어야 할 거 같아요.
element: 'li' | 'button'; | ||
idx: number; | ||
text: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P5: 타입 분리 파일 깔끔쓰..
text, | ||
}: ICreateTodo) => { | ||
const $element: any = document.createElement(element); | ||
////// :HTMLElement 를 주면 Type 'number' is not assignable to type 'string'.ts(2322) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P5: DOM element의 dataset의 파라미터들은 string
타입이라서 그런거 아닐까요? idx
값을 string
으로 바꿔주어야 할 거 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수형 컴포넌트에서의 new 생성자 사용시, this 는 무슨 타입인가요...?
함수 컴포넌트에서 new 생성자를 사용하는 건 지양되는 행동이라 any로 매핑하거나 strict 모드를 false로 주는 등의 부차적인 행위가 필요하답니다!
todoList.setState(nextTodos); | ||
todoList.state = nextTodos; //// todoList.setState에 state=nextState 할당 있는데 왜 또해야할까 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석에 적혀 있는 대로, 이 코드는 어떤 의도일까요....
setState 할당이 필요 없어보이는데요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TodoList.ts의 setState 내부에서 새로운 값으로 할당을 해주었고 콘솔로확ㄱ인했을 때 알맞게 할당이 되었는데,
여기 App.ts 17번줄 이후에 console.log(todoList.state)를 해보면 반영이 안되어있었습니다 ㅠㅠ
왜 부모인 App에선 반영된 값을 확인하지 못하는지 궁금했습니다..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지역 변수랑 프로토타입에 프로퍼티를 추가하는 것의 차이... 아닌가요?
TodoList 내부의 setState에서 state = nextState;
가 핵심인거죠?
지역 변수는 todoList.state 이렇게 접근 불가능합니다!
todoLi.addEventListener('click', (e: MouseEvent) => { | ||
// 이벤트에 대한 타입은 MouseEvent가 맞나요? | ||
// 이후에 e.target 등으로 접근하기 위해 항상 as HTMLLiElement 처럼 단언하는 방법밖에 없을까요..? | ||
const idx = (e.target as HTMLLIElement).dataset.idx; | ||
if (idx) handleComplete(parseInt(idx)); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
click은 MouseEvent가 맞아요!
우리가 타입스크립트라고 생각해봅시다. 엘리먼트에 리스너를 달았지만, 해당 태그가 어떤 것인지는 모르겠죠?
그 이유는 이벤트 전파 때문에 발생하는 문제입니다. 단언을 하지 않으면 가드를 사용해서 타입을 좁혀주는 방법 밖에는 없어요.
아래 코드를 참조해서 진행해보죠.
<button>클릭</button>
<div>
<span>다른 테스트</span>
<input type="text" />
<span>다른 테스트2</span>
</div>
document.querySelector('button').addEventListner('click', (event: MouseEvent) => {
// 대상이 버튼 엘리먼트 하나이므로 타입 단언으로도 해결 가능
});
document.querySelector('div').addEventListner('click', (event: MouseEvent) => {
// 대상이 div 태그 뿐만 아니라 span, input 도 가능해지므로 타입 가드와 단언이 필요
});
export interface ITodoHeader { | ||
$target: HTMLElement | null; | ||
text: string; | ||
} | ||
export interface ITodoForm { | ||
$target: HTMLElement | null; | ||
onSubmit: (text: string) => void; | ||
} | ||
export interface ITodoCount { | ||
$target: HTMLElement | null; | ||
initialState: ITodo[]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 형태를 잘 보면 $target
프로퍼티는 동일하죠. 다른 값은 string: any
같아요.
이렇게 반복되는 코드는 제네릭 타입이나 상속으로 적용하면 어떨까 싶습니다.
interface ITodoBase {
$target: HTMLElement | null;
}
export interface ITodoHeader extends ITodoBase {
text: string;
}
export interface ITodoForm extends ITodoBase {
onSubmit: (text: string) => void;
}
export interface ITodoCount extends ITodoBase {
initialState: ITodo[];
}
제네릭은 한 번 적용해보세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interface ITodoBase<T> {
$target: T;
}
export interface IApp<T> extends ITodoBase<T> {
initialState: ITodo[];
}
export interface ITodoHeader<T> extends ITodoBase<T> {
text: string;
}
export interface ITodoForm<T> extends ITodoBase<T> {
onSubmit: (text: string) => void;
}
export interface ITodoCount<T> extends ITodoBase<T> {
initialState: ITodo[];
}
이렇게 타입을 선언하고
호출하는 쪽에서
export default function TodoHeader({
$target,
text,
}: ITodoHeader<HTMLElement | null>) {
이렇게 제네릭타입을 쓰면 될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가적인 질문인데,
제네릭타입에 넘길때 typeof
를 사용하는건 괜찮나요??
TodoHeader.ts 의 경우라면,
import { ITodoHeader } from '../types/todoTypes';
export default function TodoHeader({
$target,
text,
// }: ITodoHeader<HTMLElement | null>) { // 여기서 이 코드 대신
}: ITodoHeader<typeof $header>) { //이렇게 typeof 를 사용해도 될까요??
const $header = document.createElement('h1');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어라? 이 코드는 컨텍스트가 완전히 다른 것 같아요.
$header를 저렇게 선언할 수가 없을텐데...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위 코드는 제가 의도한 부분이 맞습니다!
export default function TodoHeader({
$target,
text,
}: ITodoHeader<HTMLElement | null>) {
📌 설명
VanillaJS로 구현했던 간단 Todo App을 TypeScript로 마이그레이션
👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점
함수형 컴포넌트에서의 new 생성자 사용시, this 는 무슨 타입인가요...?
App.ts 19
에서 state 할당을 또 해야 하는 이슈TodoList.ts 에서 이벤트에 대한 타입은 MouseEvent가 맞나요? , 이후에 e.target 등으로 접근하기 위해 항상 as HTMLLiElement 처럼 단언하는 방법밖에 없을까요..?
types/todoTypes.ts
에 인터페이스 선언을 모아두었는데, 각 컴포넌트 파라미터별로 만들어두었습니다. 내부 속성들이 같은 경우엔 하나로 두는게 나을까요? 인터페이스 명을 어떻게 합쳐야할지에 대해 고민이 되었습니다 !