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

[현세인] TS Todo Refactor 과제 #5

Open
wants to merge 5 commits into
base: sein
Choose a base branch
from
Open

[현세인] TS Todo Refactor 과제 #5

wants to merge 5 commits into from

Conversation

imssein
Copy link
Collaborator

@imssein imssein commented Dec 10, 2023

📌 설명

FEDC5-3_VanillaJS_1 를 TypeScript로 마이그레이션

이전 PR 포인트 & 궁금한 점

✅ PR 포인트 & 궁금한 점

TS 관련 질문

  1. filter 매개변수에 대한 타입을 지정해야할까요??
const deleteTodo = (id: number) => {
  const nextState: Todo = this.state.filter((_, index: number) => index !== id);
  this.setState(nextState);
};
  1. 제네릭 타입 사용이 필요할까? 하는 의문이 있었지만, 사용 방법 및 어떤 상황에서 사용하면 좋을지에 대한 감이 안잡혀 패스했습니다. 아는 분들 있다면 알려주세욥..!
  2. 타입 추론이 가능 한 경우인데, 타입을 지정한 부분이 있을까요?

기타

  • try…catch 에서의 Error Type에 대해 공부했습니다.
  • 클래스를 이용하지않아 발생하는 에러는 */* eslint-disable @typescript-eslint/no-explicit-any */ "noImplicitAny": false*를 사용하여 제거했습니다.

코드리뷰 반영 사항

@imssein imssein self-assigned this Dec 10, 2023
Copy link
Collaborator

@bgyoons bgyoons left a comment

Choose a reason for hiding this comment

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

세인님~!
타입스크립트로 마이그레이션 하시느라 고생 많으셨습니다!
재사용성에 대해 항상 많이 배우고 가는 것 같아요 ㅎㅎ
리뷰는 아는 선에서 많이 달아보았는데 저도 타입스크립트를 너무 어려워하고 있어서 틀린 것이 있을 수 있으니 의견 많이 달아주시면 감사하겠습니다 :D

세인/Todo-App/.prettierrc Show resolved Hide resolved
세인/Todo-App/package.json Show resolved Hide resolved
세인/Todo-App/src/storage.ts Outdated Show resolved Hide resolved
세인/Todo-App/src/storage.ts Outdated Show resolved Hide resolved
세인/Todo-App/src/main.ts Show resolved Hide resolved
세인/Todo-App/src/App.ts Outdated Show resolved Hide resolved
세인/Todo-App/src/types/todo.ts Outdated Show resolved Hide resolved
세인/Todo-App/src/types/todo.ts Show resolved Hide resolved
세인/Todo-App/src/components/Todo/TodoForm.ts Outdated Show resolved Hide resolved
세인/Todo-App/src/components/Todo/TodoList.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@HoberMin HoberMin left a comment

Choose a reason for hiding this comment

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

타입 확장성 면이나 세심한 부분에서 처리해준 코드들이 굉장히 잘 구현된 것 같아요 수고하셨습니다 👍

세인/Todo-App/src/types/todo.ts Outdated Show resolved Hide resolved
세인/Todo-App/src/App.ts Outdated Show resolved Hide resolved
세인/Todo-App/src/App.ts Outdated Show resolved Hide resolved
세인/Todo-App/src/storage.ts Outdated Show resolved Hide resolved

// state에 타입을 지정해야할까?
// getItem으로 얻어지는 데이터를 통해 타입 추론이 되기때문에 필요 없는건가?
this.state = getItem(storageKey, [])

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.stateany로 평가되기 때문에 Type Safely하다고 보기는 어려울 것 같습니다.

Copy link

@evan-moon evan-moon Dec 13, 2023

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.statethis.setState를 사용한 모든 연산 과정에서 이 값들이 합성되는 순간, 결과값 또한 any로 평가되기 때문에 어플리케이션 전체의 타입 안정성을 떨어트릴 수 있으니 주의해주세요! (any가 한 군데 있다면 마치 암처럼 어플리케이션 전체로 퍼져나간다고 봐도 좋슴다)

}

export interface CommonProps {
$target: HTMLDivElement

Choose a reason for hiding this comment

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

Common~이라는 이름을 가지기에는 $target의 타입이 너무 구체적이라는 느낌이 듭니다.

컴포넌트 내부의 $targetdiv 엘리먼트가 아닐 수도 있는데, 이렇게 선언해두면 추후 어플리케이션의 확장에 방해가 될 수도 있을 것 같아요.

차라리

interface CommonProps<E extends HTMLElement = HTMLElement> {
  $target: E;
}

처럼 다형성을 확보해주면 어떨까요????

현재 어플리케이션 내에서는 이미 E = HTMLDivElement로 통일되어있었으니, 기본 타입 변수로 HTMLElement를 넣어줘도 큰 문제는 없을 것 같습니다.

어차피 HTMlDivElementHTMLElement를 상속받은 인터페이스이지만 추가로 정의된 프로퍼티가 거의 없으므로 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({

Choose a reason for hiding this comment

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

new 키워드를 써서 호출하면 contructor에 대한 추론이 안되어서 여기도 any로 평가되겠군요....음.....

Choose a reason for hiding this comment

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

이 글을 보니 직접 함수의 prototype과 constructor를 선언해서 오버라이딩하는 방식으로 덮어씌우네요.

맘에 안들긴 하지만...TS가 Prototype을 사용한 객체 생성에 대한 지원이 약하다보니 이런 편법을 사용해야할 것 같습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants