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

[3주차] 김주현 미션 제출합니다. #24

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

corinthionia
Copy link
Member

@corinthionia corinthionia commented Apr 1, 2022

안녕하세요 여러분 🙌🏻 프짱입니다. 배포 링크
저도 최근엔 JS만 사용해서 그런지 TS가 굉장히 낯설었네요... 언젠간 익숙해지는 날이 오겠죠? 😇

TypeScript 적용하기

자칫하면 anyScript가 될 뻔했지만 다행히도 모든 any를 바꿀 수 있었습니다.

Custom hooks 사용하기

입력 텍스트를 핸들링하는 useInput과 로컬스토리지 관련 함수들을 모아놓은 useLocalStorage hook을 만들어 봤습니다.

Context API 사용하기

모든 아이템들이 저장된 itemListcontext를 이용해 관리했습니다.


이번주에 제가 확진이 되는 바람에... 컨디션이 좋지 않아서... 더 많은 부분을 리팩토링하지 못한 게 조금 아쉬웠던 과제였습니다. 코드 리뷰 기다리고 있겠습니다~!

Copy link

@S-J-Kim S-J-Kim left a comment

Choose a reason for hiding this comment

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

프짱님 안녕하세요

멘토 김선종입니다.

사실 프짱님정도의 수준에 도달한 사람에게 코드의 특정 부분에 코멘트하는 것은 크게 의미가 없는 일입니다. 이쯤되면 거의 개인의 코딩 스타일 차이이기 때문이죠. 그래서 이번엔 제가 이번주동안 공부한 내용을 나누고, 개인적으로 리팩토링을 어떻게 하면 좋을지에 대해 적어보았습니다. 더 좋은 의견이 있으면 꼭 달아주세요. 함께 성장합시다........^^.....

과제 하느라 고생하셨습니다

const { inputText, handleInputChange, resetInputText } = useInput('');
const { setItemListHandler } = useContext(ItemListContext);

const handleFormSubmit = (e: React.SyntheticEvent) => {
Copy link

@S-J-Kim S-J-Kim Apr 2, 2022

Choose a reason for hiding this comment

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

Suggested change
const handleFormSubmit = (e: React.SyntheticEvent) => {
const handleFormSubmit = (e: React.FormEvent<HTMLFormElement>) => {

제가 지난주에 현재님 리뷰에 React.SyntheticEvent라는 것이 있습니다~ 라고 리뷰를 달아서 그런건지는 몰라도, 많은 분들이 이번 과제에서 이벤트 객체에 React.SyntheticEvent를 많이 사용하셨더라구요.

결론부터 말씀드리면 지양하는게 좋습니다.
저도 지난주에는 이벤트에 대해 잘 이해하지 못한 상태에서 리뷰를 달다 보니, 저 이벤트를 사용할 수 있다고 말씀을 드렸었네요. 그래서 이번주에는 해당 내용에 대해 조금 더 공부를 해보았습니다.

리액트에서 이벤트 처리를 할 때, 이벤트 핸들러에 전달받는 이벤트 객체는 자바스크립트의 네이티브 이벤트 객체가 아닙니다, SyntheticEvent 객체가 전달이 됩니다.

왜 그럴까요? SPA의 특성상 사용자와의 인터랙션이 빈번하게 발생합니다. 많은 이벤트가 발생한다는 의미죠. 하지만 그럴 때 마다 이벤트 객체를 매번 생성하게 된다면 메모리 공간과 오버헤드가 영향을 미칠 수 있겠죠. 그래서 리액트는 이벤트가 발생할 때 마다 SyntheticEvent 객체를 재사용하여 (기존에 생성되어있던 객체를 nullify하여) 이벤트를 처리하여 퍼포먼스를 향상시킵니다. (이벤트 풀링이라고도 합니다)

그래서 리액트에서는 어떤 이벤트가 발생하던 간에, SyntheticEvent 객체를 호출하고, 여기에 객체의 속성으로 원래 네이티브 이벤트를 알려주는 방식을 사용합니다. 감이 오시나요? SyntheticEvent를 사용하는 것은 결국 어떤 이벤트가 와도 괜찮다고 말하는 것과 같죠.

따라서, 만약 다중 이벤트를 폼에서 처리해야 한다면, 해당하는 이벤트들 끼리 묶어 하나의 커스텀 타입을 만드는 것이 더욱 좋은 방법일 것 같습니다.

SyntheticEvent

Comment on lines +1 to +15
import { IItem } from '../types/types';

const useLocalStorage = () => {
const getItemsFromLocalStorage = () => {
return JSON.parse(localStorage.getItem('itemList'));
};

const setItemstoLocalStorage = (itemList: IItem[]) => {
return localStorage.setItem('itemList', JSON.stringify(itemList));
};

return { getItemsFromLocalStorage, setItemstoLocalStorage };
};

export default useLocalStorage;
Copy link

@S-J-Kim S-J-Kim Apr 2, 2022

Choose a reason for hiding this comment

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

// useSyncedState.ts

import { useState } from 'react';
import useLocalStorage from 'hooks';

const useSyncedState = <T>(key: string, initialValue: T | undefined = undefined) => {
  const { getItemsFromLocalStorage, setItemstoLocalStorage } = useLocalStorage();
  const [storedValue, setStoredValue] = useState<T>(initialValue);

  useEffect(() => {
    inititalValue ? 
      setItemstoLocalStorage(key, storedValue);
      : setStoredValue(getItemsFromLocalStorage());
  }, []);

  useEffect(() => {
    setItemstoLocalStorage(key, storedValue);
  }, [storedValue]);

  return [storedValue, setStoredValue];
}

이렇게 custom hooks를 만들어 코드 내에서 useState를 대체해보면 어떨까요? useLocalStorage를 사용하는 상태관리 로직을 한번 더 추상화 해볼 수 있겠네요. useSyncedState를 사용하면 상태 관리에 localStorage의 저장 여부를 고려하지 않아도 됩니다.

Comment on lines +1 to +9
export interface IItem {
id: number;
text: string;
isDone: boolean;
}

export interface IIsDoneList {
isDoneList: boolean;
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
export interface IItem {
id: number;
text: string;
isDone: boolean;
}
export interface IIsDoneList {
isDoneList: boolean;
}
export interface IItem {
id: number;
text: string;
isDone: boolean;
}
export type IItemList = IItem[]
export interface IIsDoneList {
isDoneList: boolean;
}

IItem[] 형태가 많이 쓰이는데, 이러면 type alias를 쓰는게 좋겠죠

Copy link

@siwonblue siwonblue left a comment

Choose a reason for hiding this comment

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

안녕하세요. 주현님.
공부하려고 읽다가 몇 자 남겨봤어요.
역시 최고네요 많이 배우고 갑니당

setInputText('');
};

return { inputText, handleInputChange, resetInputText };

Choose a reason for hiding this comment

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

object 로 반환하는 이유가 궁금했는데 찾아보니 array 와 object 를 사용하는 기준이 명확하게 있네요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

이따 스터디 때 그 기준에 대해서 공유하면 좋을 것 같아요~

Comment on lines +14 to +15
<ItemList isDoneList={false} />
<ItemList isDoneList={true} />

Choose a reason for hiding this comment

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

같은 컴포넌트로 할 일 목록, 완료 목록을 구분해서 생산성이 더 높은 것 같아요
저는 구분해서 두 개의 컴포넌트를 만들었는데 이런 식으로 만드니 더 좋네요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

항상 어려운 건 설계 단계인 것 같네요
저도 많이 배워갑니다~

아래는 읽어보면 좋을 것 같은 링크입니다~
리액트 설계 가이드
컴포넌트 분리


const useLocalStorage = () => {
const getItemsFromLocalStorage = () => {
return JSON.parse(localStorage.getItem('itemList'));

Choose a reason for hiding this comment

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

저랑 대헌님이 JSON.parse 는 string 만 받아줘야 한다는 문제때문에
이 상태 그대로 사용하면 오류가 났는데 프짱님은 괜찮은 이유가 뭘까요? 흐름을 따라가봤는데 잘 못찾겠네요

Copy link
Collaborator

@itsnowkim itsnowkim left a comment

Choose a reason for hiding this comment

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

쭉 한번 훑어봤는데 단단한 코드네요
역시 제가 팀원 보는 안목이 있었습니다 ^^
앞으로도 화이팅해봅시당
최곱니다~!~!

Comment on lines +14 to +15
<ItemList isDoneList={false} />
<ItemList isDoneList={true} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

항상 어려운 건 설계 단계인 것 같네요
저도 많이 배워갑니다~

아래는 읽어보면 좋을 것 같은 링크입니다~
리액트 설계 가이드
컴포넌트 분리

handleDeleteBtnClick,
}) => {
return (
<ItemWrapper key={id}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

아 여기서 key값을 줘도 되는군요 처음 알았네요

Comment on lines +15 to +26
const handleTextClick = (e: React.MouseEvent<HTMLButtonElement>) => {
const textElement: HTMLButtonElement = e.currentTarget;

const newList = (filteredList: IItem[]) =>
filteredList.map((item: IItem) =>
item.id === parseInt(textElement.id)
? { ...item, isDone: !item.isDone }
: item
);

setItemListHandler(newList);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분 되게 신기하게 작성하셨네요
두 번 읽고 이해했습니다...
대단하시네용

setInputText('');
};

return { inputText, handleInputChange, resetInputText };
Copy link
Collaborator

Choose a reason for hiding this comment

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

이따 스터디 때 그 기준에 대해서 공유하면 좋을 것 같아요~

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

Successfully merging this pull request may close these issues.

4 participants