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

[17팀 이재혁][Chapter 1-3] React, Beyond the Basics #2

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jhlee0409
Copy link

@jhlee0409 jhlee0409 commented Oct 6, 2024

과제 체크포인트

기본과제

  • shallowEquals 구현 완료
  • deepEquals 구현 완료
  • memo 구현 완료
  • deepMemo 구현 완료
  • useRef 구현 완료
  • useMemo 구현 완료
  • useDeepMemo 구현 완료
  • useCallback 구현 완료

심화 과제

  • 기본과제에서 작성한 hook을 이용하여 렌더링 최적화를 진행하였다.
  • Context 코드를 개선하여 렌더링을 최소화하였다.

추가된 테스트

advanced.test.tsx 에만 작성

  • Context API 테스트
    • NotificationContext API 테스트
      • NotificationContext가 Provider 내부에 있는지 확인합니다.
      • NotificationContext가 Provider 내부에 없으면 에러가 발생합니다.
    • UserContext API 테스트
      • UserContext가 Provider 내부에 있는지 확인합니다.
      • UserContext가 Provider 내부에 없으면 에러가 발생합니다.
    • ThemeContext API 테스트
      • ThemeContext가 Provider 내부에 있는지 확인합니다.
      • ThemeContext가 Provider 내부에 없으면 에러가 발생합니다.
      • toggleTheme이 잘 동작하는지 확인합니다.
    • ProductsContext API 테스트
      • ProductsContext가 Provider 내부에 있는지 확인합니다.
      • ProductsContext가 Provider 내부에 없으면 에러가 발생합니다.
  • NotificationSystem 컴포넌트 테스트
    • NotificationSystem 컴포넌트가 처음에는 자식이 없는 상태로 렌더링되어야 한다
    • notification 추가 시 자식이 추가되어야 한다
    • notification에 설정되지 않은 type이 들어올 경우 info를 기본으로 보여줍니다.
  • ProductAddForm 컴포넌트 테스트
    • ProductAddForm 컴포넌트의 체크박스가 잘 동작하는지 확인합니다
    • ProductAddForm 컴포넌트의 Input이 잘 동작하는지 확인합니다
    • ProductAddForm 컴포넌트에서 submit시 product가 잘 추가되는지 확인합니다

기타

찝찝한 커버리지 100%

image

리뷰 받고 싶은 내용

  • provider/context 폴더 구조 저대로 괜찮은가? 무언가 좀 더 관리하기 좋은 구조가 있을까?
  • 추가한 테스트 코드에서 원하는 바를 정확하게 캐치하지 못할 가능성이 있는 케이스들이 존재 하는지?

궁금한 점

  • 커버리지 100% 도전 중 main.tsx 에 대해서만 에러가 나서 해결을 했으나 동작 방식의 의문이 듭니다.
  • 그 이전에 main.tsx를 테스트하는 것이 의미가 있을까요?
  • 또한 vitest providerv8instanbul 로 나눴을 때도 테스트 결과가 다르게 나오는데 차이점이나 장단점을 보아도 확실한 예제가 없어 이해하지 못했습니다.

상황

// advanced.test.tsx
const renderRoot = async () => {
  document.body.innerHTML = "<div id='root'></div>";
  await import("../main");
};
renderRoot();
document.body.innerHTML = "";
  • 위와 같은 코드일 때 아래의 이미지와 같은 에러를 맞닥뜨립니다.

image

해결

  • 이후 Unhandled 라는 키워드가 눈에 띄어서 catch 메소드 체이닝을 통해서 에러를 찍어보니 통과는 되었으나 에러를 던저는 것에서 로그를 찍는 것으로 바꿨을 뿐 근본적인 원인은 해결을 못한 상태입니다.

의문?

  • 정확히 어떤 이유 때문일까요?
  • node 환경에서 client 메소드를 호출하는 것이 문제가 되었을까요? import { createRoot } from "react-dom/client";
// advanced.test.tsx
const renderRoot = async () => {
  document.body.innerHTML = "<div id='root'></div>";
  await import("../main").catch((err) => {
    console.log(err);
  });
};
renderRoot();
document.body.innerHTML = "";

image

과제를 수행하면서 느낀점

과제 시작 전 생각

이번에도 쉽지 않겠구나 라고 생각했습니다.

과제 제출 후 생각

코치님의 친절한? 자료들이 많아서 참고하여 원활하게 진행했습니다.

기타

과제 난이도

2.5점

@JunilHwang JunilHwang closed this Oct 6, 2024
@JunilHwang JunilHwang reopened this Oct 6, 2024
Copy link

@soyoonJ soyoonJ left a comment

Choose a reason for hiding this comment

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

재혁님 이번주도 수고하셨습니다! 과제 제출 빠르게 하시고도 추가적으로 커버리지도 신경쓰시고 타입도 어떻게 하면 더 잘 선언할 수 있을 지 고민해주신 흔적이 보여서 코드리뷰 하는 내내 재밌었습니다!
equalities 파트에서 깔끔하게 구현해주셔서 제 로직도 다시 한 번 생각해보는 계기가 됐고, 호기심을 자극하는 부분이 있어서 코멘트를 몇 개 남겨두었어요! 항상 좋은 자극을 주셔서 감사합니다 :)

Comment on lines +15 to +19
const _objA = objA as Record<string, unknown>;
const _objB = objB as Record<string, unknown>;

const keysA = Object.keys(_objA);
const keysB = Object.keys(_objB);
Copy link

Choose a reason for hiding this comment

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

objA,B를 _objA,B로 한 번 더 할당해주신 이유를 알고싶어요!

Copy link
Author

Choose a reason for hiding this comment

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

Object.keys에서 Recored로 타입까지 지정해주면 지저분해져서 그냥 별도로 분리했습니다!

Comment on lines +17 to +21
const MemoizedComponent = useMemo(
() => <Component {...(prevPropsRef.current as P)} />,
// eslint-disable-next-line react-hooks/exhaustive-deps
[prevPropsRef.current]
);
Copy link

Choose a reason for hiding this comment

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

이것은 자그마한 질문인데요! memo를 사용하면 이미 다른 부분이 있을 경우에 업데이트를 하는 건데 useMemo를 추가로 사용하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

props의 값이 변하지 않는 한 메모라이즈된 컴포넌트가 그대로 반환이 되어야 한다고 생각해서 useMemo를 사용했습니다!

또 다른 방법이 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

ref 사용시
render time: 506.908935546875 ms
render time: 415.1611328125 ms
render time: 402.382080078125 ms
render time: 403.324951171875 ms
render time: 345.410888671875 ms
render time: 387.3798828125 ms
render time: 370.128173828125 ms
render time: 354.89892578125 ms
render time: 367.298095703125 ms
render time: 358.739013671875 ms
render time: 373.94091796875 ms
//평균 렌더링 시간은 약 389.60 ms입니다.

useMemo 사용시
render time: 476.381103515625 ms
render time: 420.4599609375 ms
render time: 415.77197265625 ms
render time: 408.4189453125 ms
render time: 387.155029296875 ms
render time: 381.88818359375 ms
render time: 402.846923828125 ms
render time: 372.149169921875 ms
render time: 374.06103515625 ms
render time: 364.540771484375 ms
render time: 388.998046875 ms
//평균 렌더링 시간의 평균은 약 399.33 ms입니다.

뭔가 측정하기 어렵군요

Copy link
Author

Choose a reason for hiding this comment

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

image

Copy link

Choose a reason for hiding this comment

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

멘토링 시간에 같이 질문 주셔서 감사합니다!! 덕분에 궁금증이 해소됐어요 👍


export const ProductsProvider = ({ children }: ProductsProviderProps) => {
const [filter, setFilter] = useState("");
const [products, setProducts] = useState(() => generateItems(100000));
Copy link

Choose a reason for hiding this comment

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

오 여기에서 useState(generateItems(100000));로 하지 않고 useState(() => generateItems(100000));로 작성하신 이유가 있을까요?

Copy link
Author

@jhlee0409 jhlee0409 Oct 10, 2024

Choose a reason for hiding this comment

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

그렇게 되면 ProductsProvider가 리렌더링 될때마다 매번 새로 호출하여 초기값으로 할당이 됩니다.
그러나 저렇게 함수의 형태로 넘기게되면 최초 초기화 시에만 호출을 하고 이후 리렌더링 때는 무시되는 걸로 알고 있습니다!

Copy link
Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

오옹! 이런 방법도 있군요!! 좋은 아이디어 공유 감사합니다 :)

Choose a reason for hiding this comment

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

오 이런 방법도 있군요,,, 공식문서 같이 공유해주셔서 감사합니다 !

@Yoo2
Copy link

Yoo2 commented Oct 10, 2024

코드분리가 인상적이네요!

@osohyun0224
Copy link

안녕하세요 재혁님 :) 페어팀으로 뵙게 된 18팀 오소현입니다~!
커버리지 100% 도전까지 bb 매우 멋지네요 코드도 간결하고 ,, 고민의 흔적이 많이보여요 ,, 😲😲😲
역시,, 금요일 이번주도 과제하시느라 고생많으셨어요 다음주도 화이팅입니다💪🏻💪🏻💪🏻

Comment on lines +2 to +13
if (objA === objB) {
return true;
}

if (
typeof objA !== "object" ||
typeof objB !== "object" ||
objA === null ||
objB === null
) {
return false;
}
Copy link

Choose a reason for hiding this comment

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

코드리뷰 한바퀴 돌고 오니 추가 공유 드리고 싶은 부분이 생겨 다시 찾아왔습니다!

Object.is()를 사용하면 null도 한 번에 처리가 가능하더라구요! 이 방법도 한번 스윽 제안해봅니다 🌟

Object.is() 관련문서 - https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Object/is

Copy link
Author

@jhlee0409 jhlee0409 Oct 11, 2024

Choose a reason for hiding this comment

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

오! 감사합니다
둘이 잘 비교해서 써봐야겠네요

동등 비교 메서드 끼리 비교한 표도 있네요!!

https://developer.mozilla.org/ko/docs/Web/JavaScript/Equality_comparisons_and_sameness#%EB%8F%99%EB%93%B1_%EB%A9%94%EC%84%9C%EB%93%9C_%EB%B9%84%EA%B5%90

Copy link
Author

Choose a reason for hiding this comment

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

부호가 붙은 0과 NaN 의 비교를 제외하고는 === 와 같네요!

Copy link

Choose a reason for hiding this comment

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

생각해보니 null 확인은 object type의 예외를 두기 위함이었군요! 추가 자료 공유 감사합니다 재혁님 :)

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.

5 participants