-
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 과제 #5
base: sein
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.
세인님~!
타입스크립트로 마이그레이션 하시느라 고생 많으셨습니다!
재사용성에 대해 항상 많이 배우고 가는 것 같아요 ㅎㅎ
리뷰는 아는 선에서 많이 달아보았는데 저도 타입스크립트를 너무 어려워하고 있어서 틀린 것이 있을 수 있으니 의견 많이 달아주시면 감사하겠습니다 :D
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.
타입 확장성 면이나 세심한 부분에서 처리해준 코드들이 굉장히 잘 구현된 것 같아요 수고하셨습니다 👍
|
||
// state에 타입을 지정해야할까? | ||
// getItem으로 얻어지는 데이터를 통해 타입 추론이 되기때문에 필요 없는건가? | ||
this.state = getItem(storageKey, []) |
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.
this가 any로 평가되다보니, this.state
, this.setState
등도 모두 any
로 평가되고 있네요.
이 상태라면 getItem
함수에 제네릭을 적용하여 T
를 반환하도록 선언해도 결과적으로 this.state
는 any
로 평가되기 때문에 Type Safely하다고 보기는 어려울 것 같습니다.
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.
TS가 문맥 상 this
를 추론할 수 있는 ThisType
을 제공해주기는 하지만, 이건 { fn: () => T }
와 같이 this
역할을 할 객체가 하나 존재하는 경우에만 사용할 수 있어서 이 경우에는 조금 특수한 방법으로 타입 안정성을 보장해줘야 합니다.
type ThisApp = {
state: Todo; // 사실 상 TodoItem[] <-으로 선언해도 문제없음
setState: (nextState: Todo) => void;
}
function App<T extends ThisApp>(this: T, { $target }: CommonProps) {
// 이제 this.state는 Todo로,
// this.setState는 Todo => void로 추론된다.
}
이런 부분을 그냥 any
로 넘기는 경우, this.state
나 this.setState
를 사용한 모든 연산 과정에서 이 값들이 합성되는 순간, 결과값 또한 any로 평가되기 때문에 어플리케이션 전체의 타입 안정성을 떨어트릴 수 있으니 주의해주세요! (any
가 한 군데 있다면 마치 암처럼 어플리케이션 전체로 퍼져나간다고 봐도 좋슴다)
세인/Todo-App/src/types/todo.ts
Outdated
} | ||
|
||
export interface CommonProps { | ||
$target: HTMLDivElement |
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.
Common~
이라는 이름을 가지기에는 $target
의 타입이 너무 구체적이라는 느낌이 듭니다.
컴포넌트 내부의 $target
은 div
엘리먼트가 아닐 수도 있는데, 이렇게 선언해두면 추후 어플리케이션의 확장에 방해가 될 수도 있을 것 같아요.
차라리
interface CommonProps<E extends HTMLElement = HTMLElement> {
$target: E;
}
처럼 다형성을 확보해주면 어떨까요????
현재 어플리케이션 내에서는 이미 E = HTMLDivElement
로 통일되어있었으니, 기본 타입 변수로 HTMLElement
를 넣어줘도 큰 문제는 없을 것 같습니다.
어차피 HTMlDivElement
는 HTMLElement
를 상속받은 인터페이스이지만 추가로 정의된 프로퍼티가 거의 없으므로 isomorphic하다고 봐도 무방할 것 같아요. div 뿐 아니라 일반적인 박스 모델을 정의하는 엘리먼트라면 대충 뭉개도 큰 문제는 없을 것 같습니다. (그 엘리먼트만의 특수한 기능이 없고 시맨틱한 역할만 하기 때문)
다만 form
, input
, button
과 같이 특수한 기능을 가진 엘리먼트라면 HTMLElement
가 아니라 명확한 타입 선언이 필요할 것 같아요.
이 경우
interface CommonProps<E extends HTMLElement = HTMLElement> {
$target: E;
}
type ButtonProps = CommonProps<HTMLButtonElement>;
type InputProps = CommonProps<HTMLInputElement>;
처럼 활용할 수도 있으므로 여러모로 CommonProps
는 만들어두는게 확장에 좋을 것 같기는 합니다.
createTodo | ||
}) | ||
|
||
const todoList = new TodoList({ |
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 키워드를 써서 호출하면 contructor에 대한 추론이 안되어서 여기도 any
로 평가되겠군요....음.....
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.
이 글을 보니 직접 함수의 prototype과 constructor를 선언해서 오버라이딩하는 방식으로 덮어씌우네요.
맘에 안들긴 하지만...TS가 Prototype을 사용한 객체 생성에 대한 지원이 약하다보니 이런 편법을 사용해야할 것 같습니다.
📌 설명
FEDC5-3_VanillaJS_1 를 TypeScript로 마이그레이션
이전 PR 포인트 & 궁금한 점
✅ PR 포인트 & 궁금한 점
TS 관련 질문
filter
매개변수에 대한 타입을 지정해야할까요??기타
/* eslint-disable @typescript-eslint/no-explicit-any */ "noImplicitAny": false
*를 사용하여 제거했습니다.코드리뷰 반영 사항