refactor(apply): migrate form modules to typescript #654

lokba · 2022-10-09T10:02:21Z

Resolves #595

해결하려는 문제가 무엇인가요?

form에 사용되는 컴포넌트들을 타입스크립트로 마이그레이션한다.
@common, form을 제외하고 남은 컴포넌트들을 타입스크립트로 마이그레이션한다.

어떻게 해결했나요?

1. defaultProps -> Object default values

before

const Button = ({ className, variant, cancel, type, children, ...props }) => { ... };

Button.defaultProps = {
  variant: BUTTON_VARIANT.CONTAINED,
  cancel: false,
};

after

const Button = ({
  className,
  variant = "contained",
  cancel = false,
  type,
  children,
  ...props
}: ButtonProps) => { ... };

defaultProps -> Object default values 형식으로 변환했습니다.

2. ReactDatePickerProps 대상으로 선언병합을 진행했습니다.

index.d.ts - go to definition

export interface ReactDatePickerProps<
    CustomModifierNames extends string = never,
    WithRange extends boolean | undefined = undefined,
> {
    ...
    onChange(
        date: WithRange extends false | undefined ? Date | null : [Date | null, Date | null],
        event: React.SyntheticEvent<any> | undefined,
    ): void;
    ...
}

ReactDatePickerProps의 onChange 타입은 optional 하지 않은 것을 알 수 있습니다.
현 프로젝트에서 onChange을 넘기지 않은 경우(MyPageEdit - BirthField)가 존재합니다. 이 경우에 타입에러가 발생하게 됩니다.
그리하여 선언병합을 사용해서 onChange 타입에 optional 타입을 추가하였습니다.

// date-picker.d.ts
import "react-datepicker";

/**
 * 선언병합은 왜 하였는가?
 * 1. react-datepicker는 BirthField 컴포넌트와 관련이 있다.
 * 2. BirthField의 props 중에서는 onChange가 존재한다.
 * 2. BirthField는 ReactDatePickerProps 타입을 상속받는다.
 * 3. ReactDatePickerProps 타입에서는 onChange가 optional이 아니다.
 * 4. BirthField의 onChange의 타입은 optional이어야 한다.
 * 5. 그렇기에 선언병합을 통해서 onChange 타입에 optional을 추가하였다.
 */
declare module "react-datepicker" {
  export interface ReactDatePickerProps {
    onChange?(
      date: WithRange extends false | undefined ? Date | null : [Date | null, Date | null],
      event: React.SyntheticEvent<any> | undefined
    ): void;
  }
}

왜 react-datepicker의 onChange타입은 optional하지 않은 것일까?🤔

처음에는 @types/react-datepicker의 잘못된 타입 선언이라고 생각했다. 그와 관련해서 Discussion을 올렸는데 일본의 개발자가 코멘트를 달아줬다. react-datepicker 공식문서에서 onChange는 필수적인 props이다. 그렇기에 타입 선언 또한 해당 모듈이 지향하는 디자인에 맞게 달아줘야 한다.

3. formatDateTime의 반환값때문에 타입단언을 사용했습니다. (PR #639에서 formatDateTime 함수를 수정해서 타입단언을 사용할 필요가 없어졌습니다.)

export const formatDateTime = (value: Date) => {
  const year = value.getFullYear();
  const month = value.getMonth();
  const date = value.getDate();
  const hour = value.getHours();
  const minute = value.getMinutes();

  if (isNaN(year) || isNaN(month) || isNaN(date) || isNaN(hour) || isNaN(minute)) return value;

  return `${year}-${String(month + 1).padStart(2, "0")}-${String(date).padStart(2, "0")} ${String(
    hour
  ).padStart(2, "0")}:${String(minute).padStart(2, "0")}`;
};

const RecruitmentItem = ({ ... }: RecruitmentItemProps) => {
  const formattedStartDateTime = useMemo(
    () => (recruitment.startDateTime ? formatDateTime(new Date(recruitment.startDateTime)) : ""),
    [recruitment.startDateTime]
  ) as string;

  const formattedEndDateTime = useMemo(
    () => (recruitment.endDateTime ? formatDateTime(new Date(recruitment.endDateTime)) : ""),
    [recruitment.endDateTime]
  ) as string;

  return (
       ...
        <p>
          {formattedStartDateTime} ~ {formattedEndDateTime}
        </p>
       ...
  );
};

formatDateTime 함수의 반환값 타입은 Date | string 형식이다.
이 때문에 formattedStartDateTime의 타입이 Date | string 형식으로 추론한다.
Date로 추론되기 때문에 아래와 같은 에러 메세지가 발생한다.
This JSX tag's 'children' prop expects a single child of type 'ReactNode', but multiple children were provided.
그리하여 해당 코드에서는 formattedStartDateTime과 formattedEndDateTime의 타입을 string으로 단언하였다.

어떤 부분에 집중하여 리뷰해야 할까요?

타입이 적절하게 제한되었는지 리뷰해주세요.
현재의 컨벤션에서 개선되거나 수정할 만한 부분이 존재하는지 리뷰해주세요.

RCA 룰

r: 꼭 반영해 주세요. 적극적으로 고려해 주세요. (Request changes)
c: 웬만하면 반영해 주세요. (Comment)
a: 반영해도 좋고 넘어가도 좋습니다. 그냥 사소한 의견입니다. (Approve)

soyi47

록바~ 고생하셨습니다! 👍
import 문이나 필요없는 args 제거까지 꼼꼼하게 챙겨주신 부분이 깔끔하고 멋지네요.

궁금한 점 위주로 남겼습니다. 답변 부탁드려요~

soyi47 · 2022-10-14T10:25:12Z

frontend/src/components/InquiryFloatingButton/InquiryFloatingButton.stories.tsx

+import InquiryFloatingButton from "./InquiryFloatingButton";
+
+export default {
+  title: "components/InquiryFloatingButton",
+  component: InquiryFloatingButton,
+};
+
+const Template = () => <InquiryFloatingButton />;
+
+export const Default = Template.bind({});


a: 누락된 스토리북도 챙겨주셨네요. 굿~ 👍

soyi47 · 2022-10-14T10:25:38Z

frontend/src/components/Header/Header.tsx

 import { Link, useNavigate } from "react-router-dom";
-import MemberIcon from "../../assets/icon/member-icon.svg";
-import { PATH } from "../../constants/path";
+import classNames from "classnames";
+
 import useTokenContext from "../../hooks/useTokenContext";
 import styles from "./Header.module.css";
+import { PATH } from "../../constants/path";
+import { ValueOf } from "../../../types/utility";
+import MemberIcon from "../../assets/icon/member-icon.svg";


a: import문 정리 좋네요~ 👍

soyi47 · 2022-10-14T10:27:51Z

frontend/src/components/RecruitmentItem/RecruitmentItem.tsx

+} as const;
+
+export type RecruitmentItemProps = React.HTMLAttributes<HTMLDivElement> & {
+  className?: string;
+  recruitment: Recruitment;
+  onClickButton?: React.MouseEventHandler<HTMLDivElement>;
 };


r: 타입이 최상단에 있기로 했던 것 같은데, 조금 헷갈리네요. 상수와 타입 중엔 뭘 더 위에 작성해둘까요? 🤔
저는 보통 import문 바로 아래에 타입을 적는 편이지만, 상수가 타입 선언에 사용될 수 있는 상황을 고려하면 상수가 더 상단에 있어야 할 수도 있겠네요.

동의합니다 상수가 더 위에 있어야 할 것 같아요.

디테일하게 봐주셔서 감사합니다. 반영은 했지만, 오프라인으로 해당 상황에 대해 컨벤션 논의(10월 17일)를 진행한다고 해서 논의한 결과에 따라 다시 수정이 이루어질 수도 있을 것 같습니다.

컨벤션 논의 결과, 타입 선언 위치는 최상단으로 하되 상황에 따라 유연하게 가져가자.
타입을 최상단으로 끌어올렸습니다. 반영했습니다. e00e333

soyi47 · 2022-10-14T10:32:00Z

frontend/src/components/ScrollToTop/ScrollToTop.tsx


-const ScrollToTop = ({ children }) => {
+type ScrollToTopProps = {
+  children: JSX.Element;


r: common component migration에서는 children에 React.ReactNode 타입을 줬는데요. 여기서만 JSX.Element를 준 이유가 있을까요? 특별한 이유가 없다면 통일하는 게 좋을 것 같습니다!

언제 어떤걸 쓸건지 정리한번 가시죠.

현재 JSX.Element 사용하는 곳

MessageTextInput.tsx

Form.tsx

ScrollToTop.tsx

useModalContext.tsx

현재 React.Node 사용하는 곳

Container

Description

Label

ModalPortal

Panel

TabItem

useModalContext

좋은 의견 감사합니다. ~~해당 부분도 컨벤션 논의(10월 17일)를 한 이후에 작업하도록 하겠습니다.~~

컨벤션 논의 결과, 공통 컴포넌트인 경우에는 React.ReactNode를 가져가되, 그 외 컴포넌트에서는 children에 대한 타입을 좁힐 수 있는 방향으로 하자.

common component migration에서는 children에 React.ReactNode 타입을 줬는데요. 여기서만 JSX.Element를 준 이유가 있을까요? 특별한 이유가 없다면 통일하는 게 좋을 것 같습니다!

현재 ScrollToTop 컴포넌트 사용처를 보았을 때, ReactElement만을 넘겨주기 때문에 JSX.Element로 타입을 정하였습니다.

soyi47 · 2022-10-14T10:45:43Z

frontend/src/date-picker.d.ts

+/**
+ * 선언병합은 왜 하였는가?
+ * 1. react-datepicker는 BirthField 컴포넌트와 관련이 있다.
+ * 2. BirthField의 props 중에서는 onChange가 존재한다.
+ * 2. BirthField는 ReactDatePickerProps 타입을 상속받는다.
+ * 3. ReactDatePickerProps 타입에서는 onChange가 optional이 아니다.
+ * 4. BirthField의 onChange의 타입은 optional이어야 한다.
+ * 5. 그렇기에 선언병합을 통해서 onChange 타입에 optional을 추가하였다.
+ */
+declare module "react-datepicker" {
+  export interface ReactDatePickerProps {
+    onChange?(
+      date: WithRange extends false | undefined ? Date | null : [Date | null, Date | null],
+      event: React.SyntheticEvent<any> | undefined
+    ): void;
+  }
+}


c: 잘 몰라서 질문합니다.
BirthFieldProps를 작성할 때, value 속성에 대해서는 Omit으로 BirthFieldProps에서 해당 속성을 제외시켰다가 intersection 연산으로 다시 value에 원하는 타입 지정해서 추가하는 방식으로 작성하셨는데요.
onChange 속성의 타입을 변경할 때는 선언 병합을 사용한 이유가 있나요?

c: 선언병합을 실제로 도입해보고 싶은 시도였던 것 같은데요 ㅋㅋ 저는 한가지 방식으로 통일하는 것이 좋아보이네요 :)

좋은 질문 감사합니다. 나름대로 근거가 있었습니다. 내용이 길어질 수 있지만, 천천히 따라와주시면 이해가 되실 겁니다.

type BirthFieldProps = ReactDatePickerProps; const BirthField = ({ value = null, onChange, required = false, className, name, ...props}: BirthFieldProps) {...}

BirthField 컴포넌트의 props를 한번 보실까요? 그 중에서도 value가 핵심입니다. 이 value는 어떤 역할을 할까요?
신기하게도 타입스크립트 컴파일러와 코드 레벨에서 value는 다른 역할을 하고 있습니다.

타입스크립트 컴파일러 입장

export interface ReactDatePickerProps { ... value?: string | undefined; }

ReactDatePickerProps에 value라는 타입이 정의되어 있구나. BirthField props에 있는 value는 DatePicker의 value값으로 들어옴이 틀림이 없어!!

코드 레벨

<DatePicker ... selected={value} {...props} />

BirthField props에 있는 value는 사실상 selected에 들어가는 값이구나!

결론

타입스크립트 컴파일러 관점에서는 value를 DatePicker에 value값으로 들어오는 props로 인식을 하고,
코드 레벨에서는 value는 DatePicker에 selected의 값으로 들어오는 props로 인식을 합니다.

그렇기에, 타입스크립트 컴파일러에게 BirthField props의 value는 ReactDatePickerProps에 있는 타입이 아닌 사용자 정의 타입으로 정의해야 한다고 생각합니다.

type BirthFieldProps = Omit<ReactDatePickerProps, "value"> & { value?: Date | null; };

더 쉬운 방법은 없는가?

사실 개발자가 의도하는 BirthField의 value prop은 결국 selected의 값이기에, value의 네이밍을 selected or selectedValue로 하게 되면 가장 깔끔합니다.

결국 타입을 수정하냐? 코드를 수정하냐?의 차이인데, 저는 타입을 수정하는 방향으로 가져갔습니다.

혹시 잘못된 의견이 있으면 피드백 주세요!!

질문

BirthField 컴포넌트가 prop으로 value를 받는다. 이때 value는 string | undefined.

BirthField에서는 DatePicker 컴포넌트의 selected에 (prop으로 내려받은) value를 넘겨준다.

위와 같이 말씀하신 상황을 파악했습니다.

이미 string | undefined와 Date | null 의 차이만으로도 타입을 수정하신 방향에 대해서는 동의합니다. BirthField의 value 네이밍을 selected와 같이 수정하는 방향이 가장 깔끔하다는 말씀에도 동의하고요. 하지만 말씀하신 부분에서 궁금한 부분이 있는데요.

ReactDatePickerProps에 value라는 타입이 정의되어 있구나. BirthField props에 있는 value는 DatePicker의 value값으로 들어옴이 틀림이 없어!!

왜 타입스크립트 컴파일러가 value를 꼭 DatePicker에 들어갈 props로 인식한다고 생각하시는지 잘 이해가 안 갑니다. 그냥 그와 같은 타입을 쓰겠다는 것뿐이지 DatePicker 컴포넌트에 value prop에 꼭 들어갈 것이라는 규칙은 없다고 생각해서요.

다시 질문...

사실,,, 제 질문은 value 속성의 타입을 왜 수정했냐가 아니라, value 속성에서 타입을 수정하는 방식과 달리 onChange 속성의 타입 변경에서는 선언 병합을 사용한 이유가 무엇인가요? 였답니다. 이 부분도 한번 말씀해주실 수 있을까요?

록바와의 오프라인 논의로 궁금증 해결했습니다.
각 속성의 타입 변경이 필요한 이유가 달랐네요.

value

ReactDatePickerProps에 정의된 selected는 Date | null | undefined, value는 string | null

이때 BirthField 컴포넌트는 value라는 prop을 받아 DatePicker의 selected로 넘겨주고 있다.

따라서 ReactDatePickerProps의 value 타입을 그대로 쓸 수 없고, DatePicker의 selected로 넘겨줄 수 있는 타입을 지정해주어야 함. 그래서 value의 경우 BirthFieldProps에서만 타입을 변경해도 충분함.

onChange

ReactDatePickerProps에 정의된 onChange는 optional 하지 않다.

기존 코드에서는 BirthField 컴포넌트는 onChange를 optional하게 받아, DatePicker에도 onChange를 optional하게 내려주고 있다.
따라서 BirthField 컴포넌트 만의 문제가 아니라, BirthField의 내부에서 사용하는 DatePicker에도 onChange를 optional하게 주기 위해서 ReactDatePickerProps 자체의 타입 변경이 필요하고, 이를 위해 선언 병합이 필요함

@lokba 친절한 설명 감사합니다!

c: 저도 뒤늦게 확인하면서 질문 🤔 onChange는 필수로 지정되어야 하진 않을까요?

//사용처 : MyPageEdit <BirthField name={MY_PAGE_EDIT_FORM_NAME.BIRTHDAY} value={new Date(userInfo?.birthday || null)} disabled />

MyPageEdit에서 BirthField를 사용하고 있고, onChange prop을 주입하고 있지 않습니다. 이 때문에 onChange 타입이 optional하도록 선언병합을 진행한 것이었습니다.
공원이 말씀하신 의도는, MyPageEdit - BirthField에 onChange을 넣는 방향이 좋을 것 같다 라는 의도로 말씀하신 걸까요?

아하ㅋㅋㅋ 어디에서 안쓰이나 했더니 MyPageEdit이 문제였군요.
폼 요소라면 onChange는 기본적으로 일관되게 가지고 있는 편이 자연스럽지 않을까 해서 의견으로 남겨보았습니다. 🙂

이 페이지에서 BirthField는 사실상 편집이 불가능한, 읽기 전용 필드인데요.

1 폼 요소로 보고 readonly로 취급하거나(현재가 사실상 요런 방식이네요)

2 애초에 폼 요소로 취급하지 않거나
의 방향을 고려해볼 수 있을 것 같아요.

지금 보니 MyPageEdit에서 생년월일은 요구사항 자체가 (경우에 따라 disabled 되었다 안되었다가 아니라) 항상 편집이 안되는 요소이기 때문에 2 방식으로 하는 게 더 깔끔하려나..? 싶기도 하고요.
그런데 현재 상태에서는 만약 2 방식을 한다 해도 수정 사항이 꽤 추가로 생길 것 같네요.
록바 의견만 추가로 알려주실 수 있는 부분이 있다면 남겨주시고, 추가 수정은 안해주셔도 될 것 같습니다!

현재는 상태를 유지해도 문제는 없지만, 수정이 필요한 포인트라고 생각합니다.
BirthField 대신 비활성화된 Input이 더 어울릴 것 같네요. 분명히 레거시가 맞지만, 우선순위가 낮다고 생각합니다.😎

KangYunHo1221

록바~ 여러 파일들을 수정하느라 고생하셨습니다 :)
코드리뷰 남겼으니 확인해주세요

KangYunHo1221 · 2022-10-16T08:36:55Z

frontend/src/components/ScrollToTop/ScrollToTop.tsx


-const ScrollToTop = ({ children }) => {
+type ScrollToTopProps = {
+  children: JSX.Element;


언제 어떤걸 쓸건지 정리한번 가시죠.

현재 JSX.Element 사용하는 곳

MessageTextInput.tsx

Form.tsx

ScrollToTop.tsx

useModalContext.tsx

현재 React.Node 사용하는 곳

Container

Description

Label

ModalPortal

Panel

TabItem

useModalContext

KangYunHo1221 · 2022-10-16T08:38:41Z

frontend/src/components/form/CheckBox/CheckBox.tsx

+  name,
+  checked = false,
+  required = false,
+  label,


c: label은 optional이 아닌데 checked 나 required처럼 default를 줘도 나쁘지않겠네요.

꼼꼼한 리뷰 감사합니다. 공통 컴포넌트에서도 label에 optional 타입을 주고 있었더라고요.
CheckBox, SummaryCheckField 컴포넌트에 반영했습니다. 7b289cd

KangYunHo1221 · 2022-10-16T08:59:54Z

frontend/src/components/Header/Header.stories.tsx

@@ -19,8 +19,6 @@ export default {
  ],
 };

-const Template = (args) => <Header {...args} />;
+const Template = () => <Header />;


r: Parameter 'Story' implicitly has an 'any' type가 발생합니다.

decorators: [ (Story) => ( <MemoryRouter initialEntries={[

타입을 지정해줘서 해결 가능하다고 합니다.

import { addDecorator } from "@storybook/react"; import { MemoryRouter } from "react-router-dom"; import Header from "./Header"; type DecoratorFunction = Parameters<typeof addDecorator>[0]; type HeaderMetaData = { component: () => JSX.Element; title: string; decorators?: DecoratorFunction[]; } const HeaderMeta: HeaderMetaData = { title: "components/Header", component: Header, decorators: [ (Story) => ( <MemoryRouter initialEntries={[ { pathname: "/'", }, ]} > <Story /> </MemoryRouter> ), ], }; export default HeaderMeta; const Template = () => <Header />; export const Default = Template.bind({});

해결참조

덕분에 쉽게 해결했습니다. 반영했습니다. 01c3eed

KangYunHo1221 · 2022-10-16T09:01:36Z

frontend/src/components/Header/Header.tsx

    setIsShowMemberMenu(target.checked);
  };

-  const routeTo = ({ pathname }) => {
+  const routeTo = ({ pathname }: { pathname: ValueOf<typeof PATH> }) => {


a: ValueOf 만든 거 잘 쓰셨네요 굳굳~

ValueOf 못참죠ㅋㅋ

KangYunHo1221 · 2022-10-16T09:07:20Z

frontend/src/date-picker.d.ts

+/**
+ * 선언병합은 왜 하였는가?
+ * 1. react-datepicker는 BirthField 컴포넌트와 관련이 있다.
+ * 2. BirthField의 props 중에서는 onChange가 존재한다.
+ * 2. BirthField는 ReactDatePickerProps 타입을 상속받는다.
+ * 3. ReactDatePickerProps 타입에서는 onChange가 optional이 아니다.
+ * 4. BirthField의 onChange의 타입은 optional이어야 한다.
+ * 5. 그렇기에 선언병합을 통해서 onChange 타입에 optional을 추가하였다.
+ */
+declare module "react-datepicker" {
+  export interface ReactDatePickerProps {
+    onChange?(
+      date: WithRange extends false | undefined ? Date | null : [Date | null, Date | null],
+      event: React.SyntheticEvent<any> | undefined
+    ): void;
+  }
+}


c: 선언병합을 실제로 도입해보고 싶은 시도였던 것 같은데요 ㅋㅋ 저는 한가지 방식으로 통일하는 것이 좋아보이네요 :)

KangYunHo1221 · 2022-10-16T09:16:49Z

frontend/src/components/form/EmailField/EmailField.tsx

-          ✓
-        </div>
-      );
+      return <div className={classNames(styles["authenticated"], styles["input-button"])}>✓</div>;


c: disabled가 제거된 이유는 무엇인가요?

<div className={classNames(styles["authenticated"], styles["input-button"])} disabled> ✓ </div>

기존 코드를 보시면, div 태그에 disabled 속성을 부여하고 있었는데요.

div 태그에 disabled 속성은 하는 역할이 없기에 제거하였습니다.

기존 disabled의 역할을 따로 부여하지 않아도 되는건가요?

pointer-events: none을 이용해서 클릭을 막던지 같은 UI는 따로 없어도 될까요?

@hwangstar156 질문 감사합니다.

disabled 제거 전후의 UI가 동일하여서, 스타일 코드를 추가하지 않아도 될 것으로 보입니다.

추측하건대, 이전에 승인된 상태에 대한 UI는 Button 요소로 이뤄질 가능성이 높고 Button에 disabled 속성을 주고 있었을 겁니다. 무슨 이유인지 모르겠지만 현재 상황처럼 Button -> div 요소로 바뀌게 되었고 그 과정에서 disabled를 빼는 것을 놓치지 않았나 싶습니다. 결론적으로 disabled는 빈 껍데기였다.

a: 요것도 기존 코드다 보니 참고 정보로만 코멘트! 사실 위 논의 흐름과 별개로, (아실 것 같지만) 버튼을 <button/> 이 아닌 태그로 만드는 것은 접근성 측면에서 권장하지 않는 방법입니다 😂 이후 수정이 필요할 수 있겠네요
https://www.youtube.com/watch?v=L1YLFYlkGnc 영상을 비롯해, 요 채널에 괜찮은 가이드 내용이 많으니 접근성과 관련해 필요하실 때 한번 참고해보셔요 :)

button으로 수정할 생각은 못한 것 같네요.😅 수정하는 과정에서 텍스트로 남기기에 이해하기 어려운 상황이 발생해서요. 오프라인으로 전달하겠습니다!!

오! 이번 PR에서 button으로 수정하면 좋을 것 같다는 의견은 아니었습니다!
TS 전환 이슈이니, 동작이나 구체적인 구현 자체가 기존과 달라지는 것은 별도 이슈로 보는 게 좋을 것 같아요 🙂

(cherry picked from commit e25f7c7)

(cherry picked from commit 952b0f6)

(cherry picked from commit 271e3ba)

(cherry picked from commit de36ba3)

(cherry picked from commit 8fff3d3)

(cherry picked from commit e2bcbba)

(cherry picked from commit 150c661)

(cherry picked from commit a2d34b3)

(cherry picked from commit 8b706ec)

(cherry picked from commit 5121663)

(cherry picked from commit 60d6ae1)

(cherry picked from commit d4fb436)

(cherry picked from commit e824e19)

(cherry picked from commit 9ea6f8d)

(cherry picked from commit 89e796b)

(cherry picked from commit 55631c6)

(cherry picked from commit b58e58c)

soyi47

록바~ 컨벤션 논의한 사항까지 꼼꼼하게 잘 챙겨주셨네요. 👍

다시 질문 남겼으니 확인 부탁드려요~

soyi47 · 2022-10-25T13:39:03Z

frontend/src/date-picker.d.ts

+/**
+ * 선언병합은 왜 하였는가?
+ * 1. react-datepicker는 BirthField 컴포넌트와 관련이 있다.
+ * 2. BirthField의 props 중에서는 onChange가 존재한다.
+ * 2. BirthField는 ReactDatePickerProps 타입을 상속받는다.
+ * 3. ReactDatePickerProps 타입에서는 onChange가 optional이 아니다.
+ * 4. BirthField의 onChange의 타입은 optional이어야 한다.
+ * 5. 그렇기에 선언병합을 통해서 onChange 타입에 optional을 추가하였다.
+ */
+declare module "react-datepicker" {
+  export interface ReactDatePickerProps {
+    onChange?(
+      date: WithRange extends false | undefined ? Date | null : [Date | null, Date | null],
+      event: React.SyntheticEvent<any> | undefined
+    ): void;
+  }
+}


질문

BirthField 컴포넌트가 prop으로 value를 받는다. 이때 value는 string | undefined.

BirthField에서는 DatePicker 컴포넌트의 selected에 (prop으로 내려받은) value를 넘겨준다.

위와 같이 말씀하신 상황을 파악했습니다.

이미 string | undefined와 Date | null 의 차이만으로도 타입을 수정하신 방향에 대해서는 동의합니다. BirthField의 value 네이밍을 selected와 같이 수정하는 방향이 가장 깔끔하다는 말씀에도 동의하고요. 하지만 말씀하신 부분에서 궁금한 부분이 있는데요.

ReactDatePickerProps에 value라는 타입이 정의되어 있구나. BirthField props에 있는 value는 DatePicker의 value값으로 들어옴이 틀림이 없어!!

왜 타입스크립트 컴파일러가 value를 꼭 DatePicker에 들어갈 props로 인식한다고 생각하시는지 잘 이해가 안 갑니다. 그냥 그와 같은 타입을 쓰겠다는 것뿐이지 DatePicker 컴포넌트에 value prop에 꼭 들어갈 것이라는 규칙은 없다고 생각해서요.

다시 질문...

사실,,, 제 질문은 value 속성의 타입을 왜 수정했냐가 아니라, value 속성에서 타입을 수정하는 방식과 달리 onChange 속성의 타입 변경에서는 선언 병합을 사용한 이유가 무엇인가요? 였답니다. 이 부분도 한번 말씀해주실 수 있을까요?

soyi47

록바 고생하셨습니다!
저는 approve 하겠습니다. 다른 분들은 어떤 리뷰 주실지 기대할게요~

soyi47 · 2022-10-27T07:24:34Z

frontend/src/date-picker.d.ts

+/**
+ * 선언병합은 왜 하였는가?
+ * 1. react-datepicker는 BirthField 컴포넌트와 관련이 있다.
+ * 2. BirthField의 props 중에서는 onChange가 존재한다.
+ * 2. BirthField는 ReactDatePickerProps 타입을 상속받는다.
+ * 3. ReactDatePickerProps 타입에서는 onChange가 optional이 아니다.
+ * 4. BirthField의 onChange의 타입은 optional이어야 한다.
+ * 5. 그렇기에 선언병합을 통해서 onChange 타입에 optional을 추가하였다.
+ */
+declare module "react-datepicker" {
+  export interface ReactDatePickerProps {
+    onChange?(
+      date: WithRange extends false | undefined ? Date | null : [Date | null, Date | null],
+      event: React.SyntheticEvent<any> | undefined
+    ): void;
+  }
+}


록바와의 오프라인 논의로 궁금증 해결했습니다.
각 속성의 타입 변경이 필요한 이유가 달랐네요.

value

ReactDatePickerProps에 정의된 selected는 Date | null | undefined, value는 string | null

이때 BirthField 컴포넌트는 value라는 prop을 받아 DatePicker의 selected로 넘겨주고 있다.

따라서 ReactDatePickerProps의 value 타입을 그대로 쓸 수 없고, DatePicker의 selected로 넘겨줄 수 있는 타입을 지정해주어야 함. 그래서 value의 경우 BirthFieldProps에서만 타입을 변경해도 충분함.

onChange

ReactDatePickerProps에 정의된 onChange는 optional 하지 않다.

기존 코드에서는 BirthField 컴포넌트는 onChange를 optional하게 받아, DatePicker에도 onChange를 optional하게 내려주고 있다.
따라서 BirthField 컴포넌트 만의 문제가 아니라, BirthField의 내부에서 사용하는 DatePicker에도 onChange를 optional하게 주기 위해서 ReactDatePickerProps 자체의 타입 변경이 필요하고, 이를 위해 선언 병합이 필요함

@lokba 친절한 설명 감사합니다!

hwangstar156 · 2022-11-01T01:29:12Z

frontend/src/components/form/EmailField/EmailField.tsx

-          ✓
-        </div>
-      );
+      return <div className={classNames(styles["authenticated"], styles["input-button"])}>✓</div>;


기존 disabled의 역할을 따로 부여하지 않아도 되는건가요?

pointer-events: none을 이용해서 클릭을 막던지 같은 UI는 따로 없어도 될까요?

hwangstar156 · 2022-11-01T01:33:38Z

frontend/src/components/form/GenderField/GenderField.tsx

+type GenderFieldProps = {
+  className?: string;
+  required?: boolean;
+  value: string;


r: value의 타입은 MALE | FEMALE로 더 좁힐 수 있지 않나요?

아주 좋았습니다. 👍 6489571

hwangstar156 · 2022-11-01T01:38:41Z

frontend/src/components/form/GenderField/GenderField.tsx

+type GenderFieldProps = {
+  className?: string;
+  required?: boolean;
+  value: string;
+  onChange: React.ChangeEventHandler<HTMLInputElement>;
+};


c: SummaryCheckField에서는 value, onChange타입들을 묶어서 {...props}로 주고 있네요. 통일하는 방안은 어떤가요?

이것도 React.InputHTMLAttributes<HTMLInputElement>로 처리할 수 있지않을까요?

hwangstar156 · 2022-11-01T01:43:04Z

frontend/src/components/RecruitmentItem/RecruitmentItem.tsx

@@ -31,8 +44,8 @@ const RecruitmentItem = ({ recruitment, onClickButton, className, ...props }) =>
  return (
    <div
      className={classNames(styles["content-wrapper"], className, active ? styles.active : "")}
-      {...props}
      onClick={active ? onClickButton : () => {}}


c: 록바 이슈가 아니긴 하지만 active && onClickButton 해도 되지않을까 싶네요..!

active && onClickButton로 수정해보니 타입 에러가 발생하네요.
active가 false인 경우, onClick 타입이 false | React.MouseEventHandler<HTMLDivElement>로 추론해서 발생하는 것 같네요. 기존 상태 유지하겠습니다. :D

KangYunHo1221

선언병합을 위한 '꺾이지 않는 마음' 좋네요 :)
approve 하겠습니다. 고생하셨어요~

greenblues1190

수고하셨습니다 록바! 파라미터 타입에 대해 rc 남겼습니다~

greenblues1190 · 2022-11-18T05:01:23Z

frontend/src/components/Header/Header.stories.tsx

+  decorators: [
+    (Story) => (
+      <MemoryRouter
+        initialEntries={[
+          {
+            pathname: "/'",
+          },
+        ]}
+      >
+        <Story />
+      </MemoryRouter>
+    ),
+  ],


r: pathname에 작은 따옴표가 잘못 들어간 것 같습니다.

수정했습니다. 479c8cc

greenblues1190 · 2022-11-18T05:10:48Z

frontend/src/components/RecruitmentItem/RecruitmentItem.stories.tsx

+  recruitment: {
+    id: 1,
+    title: "우아한테크코스 1기",
+    term: {
+      id: 1,
+      name: "이름",
+    },
+    recruitable: true,
+    hidden: false,
+    startDateTime: "2020-10-05T10:00:00",
+    endDateTime: "2020-11-05T10:00:00",
+    status: "RECRUITING",
+  },


a: 이 부분은 dummy.ts에서 가져와서 필요한 부분만 추가해도 될 것 같아요.

recruitmentDummy 데이터에는 term에 대한 property가 존재하지 않은 상태입니다.
recruitmentDummy 데이터에서 가져오고 term에 대한 부분을 주입하는 코드보다, 차라리 하드하게 현재처럼 하는 것이 좋을 것 같다는 생각이 드네요.(+ term을 주입하지 않으면 타입에러가 발생해서 주입해야 하는 상태입니다.) 어떻게 생각하실까요?🤔

dummy에서 가져오는 것이 관리포인트를 줄이는 방법이라고도 생각할 수 있을 것 같아요. 사람마다 의견차가 있을 수 있기에 a로 남겼습니다.

greenblues1190 · 2022-11-18T05:12:36Z

frontend/src/components/form/BirthField/BirthField.stories.tsx


-const Template = (args) => <BirthField {...args} />;
+const Template: ComponentStory<typeof BirthField> = (args) => <BirthField {...args} />;


a: args를 사용하지 않는 템플릿 중 어떤 것은 args를 전달하지 않고 어떤 것은 전달하는 이유는 무엇인가요?

Header.stories 같은 상황을 말씀하시는 걸까요?

네 맞습니다. 크게 중요한 부분은 아니라 a로 남겼어요~

아하 그렇군요. Header 컴포넌트의 경우 prop으로 받는 매개변수가 없기 때문에, 스토리상에서도 args를 전달하지 않아도 무방하다고 생각해서 제거했습니다. :D

greenblues1190 · 2022-11-18T05:15:17Z

frontend/src/components/form/BirthField/BirthField.tsx

 import styles from "./BirthField.module.css";

 registerLocale("ko", ko);

-const BirthField = ({ value, onChange, required, className, ...props }) => {
+type BirthFieldProps = Omit<ReactDatePickerProps, "value"> & {
+  value?: Date | null;


r: 디폴트 파라미터로 null을 할당하기 때문에 옵셔널 연산자는 제거하는게 어떨까요?

Suggested change

value?: Date | null;

value: Date | null;

#654 (comment)

greenblues1190 · 2022-11-18T05:18:21Z

frontend/src/components/form/CheckBox/CheckBox.tsx

 import Label from "../../@common/Label/Label";
 import styles from "./CheckBox.module.css";

-const CheckBox = ({ label, name, checked, onChange, required, ...props }) => {
+type CheckBoxProps = React.InputHTMLAttributes<HTMLInputElement> & {
+  label?: string;


r: 디폴트 파라미터이기 때문에 옵셔널 제거해도 될 것 같습니다.

Suggested change

label?: string;

label: string;

음...디폴트 파라미터로 할당하기 때문에 optional로 지정하는게 맞지 않나요? 👀
CheckBox 사용처에서 label에 대한 prop 지정을 하지 않을 수 있으며, 이 경우에 디폴트로 지정한 값이 매핑되는 흐름으로 진행되기에 저는 label은 optional 타입이어야 하지 않나 생각이 드는데요. 어떻게 생각하실까요?

제가 잘못 생각했네요. CheckBox 컴포넌트 내에서 옵셔널을 제거해도 타입 에러가 나오지 않길래 제거해도 된다고 생각했습니다. 옵셔널 제거 시 사용처에서 label 할당하지 않으면 에러가 출력되네요.

greenblues1190 · 2022-11-18T05:20:37Z

frontend/src/components/form/GenderField/GenderField.tsx

-const GenderField = ({ required, className, value, onChange }) => {
+type GenderFieldProps = {
+  className?: string;
+  required?: boolean;


r: 디폴트 파라미터이기 때문에 옵셔널 제거해도 될 것 같습니다.

#654 (comment)

woowapark

request changes 는 매우 소소한 것 하나만 남겨두었습니다
그 외에는 참고 사항 및 질문!
고생 많으셨습니다 🙏

woowapark · 2022-11-21T05:49:33Z

frontend/src/components/form/EmailField/EmailField.tsx

+  resetAuthenticationCode: () => void;
+  setEmailStatus: React.Dispatch<React.SetStateAction<ValueOf<typeof EMAIL_STATUS>>>;
+  setErrorMessage: (name: string, errorMessage: string) => void;
+};


a: 지금 보니 컴포넌트는 '이메일 필드' 인데, 인증 코드와 관련된 것도 함께 처리하다보니 필요한 props도 여러 가지가 섞여 있는 것처럼 보이네요. props의 수가 일정 수 이상이 된다면 필요한 것 이상의 책임을 가지고 있는 컴포넌트가 아닌지 의심해보면 좋을 것 같습니다. 이후에 실무를 하실 때에도 비슷한 경우에 리팩터링 제안을 해볼 수도 있을 것 같고요 :)
작업해주신 내용과는 별개로 기존에 있던 문제이고, PR은 타입스크립트로 변환만 하는 PR이니 컴포넌트 분리 등의 작업이 섞이면 안될 것 같아 생각거리 정도로 a 코멘트로만 남겨두어요! :)

좋은 조언 감사합니다!!

woowapark · 2022-11-21T05:56:33Z

frontend/src/components/form/EmailField/EmailField.tsx

-          ✓
-        </div>
-      );
+      return <div className={classNames(styles["authenticated"], styles["input-button"])}>✓</div>;


a: 요것도 기존 코드다 보니 참고 정보로만 코멘트! 사실 위 논의 흐름과 별개로, (아실 것 같지만) 버튼을 <button/> 이 아닌 태그로 만드는 것은 접근성 측면에서 권장하지 않는 방법입니다 😂 이후 수정이 필요할 수 있겠네요
https://www.youtube.com/watch?v=L1YLFYlkGnc 영상을 비롯해, 요 채널에 괜찮은 가이드 내용이 많으니 접근성과 관련해 필요하실 때 한번 참고해보셔요 :)

woowapark · 2022-11-21T05:58:02Z

frontend/src/components/form/Form/Form.tsx

+type FormProps = {
+  className?: string;
+  children: JSX.Element;
+  onSubmit?: React.FormEventHandler<HTMLFormElement>;


r: 폼이라면 onSubmit 정도는 기본 동작으로 필수로 받아야 하지 않을까 싶은데 어떠신가요? 🙂 사용하는 코드 내에서도 옵셔널에 대한 처리가 되어 있지 않기도 하고요.

좋은 의견 감사합니다. 그렇다면 PasswordFindResult 컴포넌트에 Form을 제거하는 방향으로 진행하면 될까요?

아하 그러네요 생김새가 유사해서 요렇게 했었던 것 같은데,
PasswordFindResult 자체는 폼 요소들을 가지고 있는 UI도 아니어서 Form을 사용하지 않아도 될 것 같습니다.
요건 타입 지정에 연결되는 부분 같아서, 괜찮다면 추가 반영 부탁드릴게요 🙏

수정했습니다. 0899eb4 355ef58
덕분에 마음이 더 편안해졌네요 :D

woowapark · 2022-11-21T06:04:44Z

frontend/src/date-picker.d.ts

+/**
+ * 선언병합은 왜 하였는가?
+ * 1. react-datepicker는 BirthField 컴포넌트와 관련이 있다.
+ * 2. BirthField의 props 중에서는 onChange가 존재한다.
+ * 2. BirthField는 ReactDatePickerProps 타입을 상속받는다.
+ * 3. ReactDatePickerProps 타입에서는 onChange가 optional이 아니다.
+ * 4. BirthField의 onChange의 타입은 optional이어야 한다.
+ * 5. 그렇기에 선언병합을 통해서 onChange 타입에 optional을 추가하였다.
+ */
+declare module "react-datepicker" {
+  export interface ReactDatePickerProps {
+    onChange?(
+      date: WithRange extends false | undefined ? Date | null : [Date | null, Date | null],
+      event: React.SyntheticEvent<any> | undefined
+    ): void;
+  }
+}


c: 저도 뒤늦게 확인하면서 질문 🤔 onChange는 필수로 지정되어야 하진 않을까요?

woowapark

고생 많으셨어요! 👏👏👏👏👏

lokba self-assigned this Oct 9, 2022

lokba requested review from KangYunHo1221, hwangstar156, woowapark, soyi47 and greenblues1190 October 9, 2022 10:06

lokba changed the title ~~refactor: migrate Form file to typescript~~ refactor(apply): migrate form modules to typescript Oct 10, 2022

lokba force-pushed the feature/typescript-form branch 2 times, most recently from 29d2b88 to c54eb98 Compare October 14, 2022 08:12

soyi47 suggested changes Oct 14, 2022

View reviewed changes

KangYunHo1221 suggested changes Oct 16, 2022

View reviewed changes

woowahan-pjs force-pushed the develop branch from a17fe1e to 2886d2d Compare October 17, 2022 06:42

lokba added 18 commits October 17, 2022 15:48

refactor: migrate Footer folder to typescript

1eb5afc

(cherry picked from commit e25f7c7)

refactor: migrate Header folder to typescript

d4870d0

(cherry picked from commit 952b0f6)

refactor: migrate PrivateRoute file to typescript

ecfb0b9

(cherry picked from commit 271e3ba)

refactor: migrate ScrollToTop file to typescript

d71b771

(cherry picked from commit de36ba3)

refactor: migrate RecruitmentItem folder to typescript

32ad0a7

(cherry picked from commit 8fff3d3)

refactor: migrate CancelButton file to typescript

4df6b7f

(cherry picked from commit e2bcbba)

refactor: migrate Form folder to typescript

0e24dce

(cherry picked from commit 150c661)

refactor: migrate GenderField folder to typescript

39ba8a2

(cherry picked from commit a2d34b3)

refactor: migrate CheckBox folder to typescript

f358fa9

(cherry picked from commit 8b706ec)

refactor: migrate SummaryCheckField folder to typescript

a0dc90a

(cherry picked from commit 5121663)

feat: add InquiryFloatingButton story

5a8153a

(cherry picked from commit 60d6ae1)

refactor: migrate EmailField file to typescript

a33c6cd

(cherry picked from commit d4fb436)

chore: install @types/react-datepicker

d2989e9

(cherry picked from commit e824e19)

refactor: declaration merging to ReactDatePickerProps

0d74bcd

(cherry picked from commit 9ea6f8d)

refactor: migrate BirthField file to typescript

d81f603

(cherry picked from commit 89e796b)

refactor: add newline

776332a

(cherry picked from commit 55631c6)

chore: add yarn.lock

778df85

(cherry picked from commit b58e58c)

refactor: remove type assertion in RecruitmentItem

5ecc692

refactor: rearrange based on type convention

e89f081

lokba force-pushed the feature/typescript-form branch from e00e333 to e89f081 Compare October 23, 2022 05:32

lokba added 3 commits October 23, 2022 15:11

refactor: add optional type and default value on label prop

7b289cd

refactor: Header.stories - add type on Paramter Story

01c3eed

refactor: BirthField - add optional type on value prop

ebcce83

lokba requested review from soyi47 and KangYunHo1221 October 24, 2022 06:29

soyi47 suggested changes Oct 25, 2022

View reviewed changes

soyi47 approved these changes Oct 27, 2022

View reviewed changes

hwangstar156 suggested changes Nov 1, 2022

View reviewed changes

refactor: narrow string to literal string union about gender type

6489571

KangYunHo1221 approved these changes Nov 9, 2022

View reviewed changes

lokba requested a review from hwangstar156 November 9, 2022 13:48

greenblues1190 suggested changes Nov 18, 2022

View reviewed changes

woowapark requested changes Nov 21, 2022

View reviewed changes

refactor: modify wrong pathname in Header.stories

479c8cc

lokba requested review from greenblues1190, soyi47 and woowapark November 22, 2022 03:10

lokba added 3 commits November 22, 2022 19:41

refactor: remove unnessary form tag in PasswordFindResult

0899eb4

refactor: change onSubmit type into required type

355ef58

refactor: remove annotation

108b41c

woowapark approved these changes Nov 28, 2022

View reviewed changes

soyi47 approved these changes Mar 2, 2023

View reviewed changes

Merge branch 'develop' into feature/typescript-form

cde08de

woowapark merged commit dde8fb3 into develop May 9, 2023

woowapark deleted the feature/typescript-form branch August 4, 2023 00:25


		const Template = (args) => <BirthField {...args} />;
		const Template: ComponentStory<typeof BirthField> = (args) => <BirthField {...args} />;

refactor(apply): migrate form modules to typescript #654

refactor(apply): migrate form modules to typescript #654

Conversation

lokba commented Oct 9, 2022 • edited Loading

해결하려는 문제가 무엇인가요?

어떻게 해결했나요?

1. defaultProps -> Object default values

before

after

2. ReactDatePickerProps 대상으로 선언병합을 진행했습니다.

index.d.ts - go to definition

왜 react-datepicker의 onChange타입은 optional하지 않은 것일까?🤔

3. formatDateTime의 반환값때문에 타입단언을 사용했습니다. (PR #639에서 formatDateTime 함수를 수정해서 타입단언을 사용할 필요가 없어졌습니다.)

어떤 부분에 집중하여 리뷰해야 할까요?

RCA 룰

soyi47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokba Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokba Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokba Oct 23, 2022 • edited Loading

Choose a reason for hiding this comment

타입스크립트 컴파일러 입장

코드 레벨

결론

더 쉬운 방법은 없는가?

혹시 잘못된 의견이 있으면 피드백 주세요!!

Choose a reason for hiding this comment

질문

다시 질문...

Choose a reason for hiding this comment

value

onChange

Choose a reason for hiding this comment

lokba Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokba Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

KangYunHo1221 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokba Oct 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokba Oct 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokba Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokba Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyi47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

질문

다시 질문...

soyi47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

value

onChange

Choose a reason for hiding this comment

lokba commented Oct 9, 2022 •

edited

Loading

lokba Oct 16, 2022 •

edited

Loading

lokba Oct 16, 2022 •

edited

Loading

lokba Oct 23, 2022 •

edited

Loading

lokba Nov 22, 2022 •

edited

Loading

lokba Nov 22, 2022 •

edited

Loading

lokba Oct 23, 2022 •

edited

Loading

lokba Oct 23, 2022 •

edited

Loading

lokba Nov 8, 2022 •

edited

Loading

lokba Nov 22, 2022 •

edited

Loading

lokba Nov 8, 2022 •

edited

Loading

lokba Nov 22, 2022 •

edited

Loading

lokba Nov 22, 2022 •

edited

Loading

lokba Nov 22, 2022 •

edited

Loading

lokba Nov 22, 2022 •

edited

Loading