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

[2단계 - 계산기] 호프(김문희) 미션 제출합니다. #46

Merged
merged 10 commits into from
Apr 26, 2022

Conversation

moonheekim0118
Copy link

@moonheekim0118 moonheekim0118 commented Apr 24, 2022

데모페이지

안녕하세요 로이님 :) 다시한번 인사드려요. 먼저 ,저번 리뷰 정말 감사했습니다. 놓쳤던 부분들을 짚어주셔서 큰 도움이 되었어요 👍 👍 👍
그럼 먼저 수정한 사항들을 간략하게 설명해드리겠습니다.

✨ 기능 오류 해결

  1. 숫자는 한번에 최대 3자리 수까지 입력 가능하다.
  • 3자리가 넘어 간 후에 숫자를 입력 할 시 마지막 자릿수를 수정해주는 식으로 해결했습니다. :)
  1. 계산 결과를 표현할 때 소수점 이하는 버림한다
  • 수정했습니다.
  1. 출력값 있는 상황에" 페이지 이탈시 이탈 여부를 확인한다.
  • 이 부분은 제가 잘 이해를 못한 것일 수도 있는데, 현재 사용자가 만약 하나라도 숫자 / 연산자를 입력 했을 경우 이탈 여부를 확인하도록 구현했는데요! 혹시라도 제 이해가 틀렸다면 말씀해주시면 감사하겠습니다 🙇

✨ 리팩토링

컴포넌트 분리

조금씩 연산이 늘다보니, Calculator 컴포넌트가 너무 복잡해지는 것 같아서, 컴포넌트를 분리해보았습니다.

  • 이전 : Calculator 컴포넌트 하나로 구현
  • 현재 : Calculator, Digits(숫자), Operations(연산자), Expression(출력값), Modifier(AC 버튼)로 분리

이벤트 위임 제거

  • 이전 : 같은 핸들러를 갖는 버튼이 여러개 있을 경우 부모 엘리먼트 에 이벤트 핸들러를 등록
  • 현재 : 각각 버튼에 이벤트 핸들러 등록
  • 이유
    • 해당 링크를 통해 알게된건데요, 리액트는 이벤트를 노드 자체에 붙이지 않고 리액트 자체가 원래 이벤트 위임을 이용하여 도큐먼트 레벨에서 이벤트를 리스닝 한다고 하더라구요. 그래서 이벤트 위임을 구현해주는 것과 그렇지 않은 것의 성능상 차이 없다고 이해했습니다.
    • 또한, 이벤트 위임으로 구현할 경우 각각의 버튼을 구분하기 위해서 (Digit 의 종류나 Operation의 종류) 어쩔수 없이 DOM 요소에 의존을 해야하는데요, 이벤트 위임을 빼주고 모든 버튼에 핸들러를 달았더니, 인자를 핸들러에 직접 넘겨줄 수 있어서 DOM 과 이벤트 핸들러의 불필요한 의존성을 제거 할 수 있었습니다.
    • 인자를 핸들러에 넘겨줄 때 처음에는 ()=> onClick(digit) 이런 식으로 새로 함수를 생성해주었는데요, 리팩토링하여 커링함수로 핸들러 함수를 선언해주어 onClick=(digit)=>()=>{..} 이러한 문제점을 해결했습니다 :)

그리고, Hooks 로 수정하면서 여러 문제점들을 마주했고, 아래와 같은 시도를 해보았습니다.

✨Hooks 관련 사항들

1. useLayoutEffect 사용

로컬스토리지에 저장된 값을 가져와서 state 에 저장해줄 때, useLayoutEffect 라는 api 를 사용했습니다. 사용한 이유는 아래와 같아요.

  • useEffect 는 DOM 이 그려지고 난 후 실행되어서 , 로컬스토리지에 저장된 값이 있더라도 아주 잠깐동안 초기 상태를 사용자에게 보여주게 됩니다.
  • 반면 useLayoutEffect 는 DOM 이 그려지기 전에 실행되어서, 위의 불편함을 해소합니다.

2. Ref 사용

사실 ref 사용하신 걸 보고 의아해 하실 수도 있는데요! 저는 아래와 같은 문제점을 해결하고자 ref 를 사용해보았어요.

마주한 문제

  • beforeunload 이벤트에 대한 핸들러를 초기 렌더링에 등록 해야 했습니다.
  • 해당 이벤트의 핸들러는계산기 상태가 초기값인지 확인하고 , 초깃값이 아니라면 (= 출력값이 있다면) 변화된 계산기 상태를 로컬스토리지에 저장 해야 했습니다.
  • 따라서 useEffect 디펜던시 배열을 빈 값으로 주고, 이벤트 등록 해주는 로직을 맨처음에만 해주면, 제대로된 상태변경 체크가 되지 않는 문제점이 있었어요. 그래서 저에게는 두가지 선택지가 있었습니다.

주어진 해결방법

  1. State 가 변경 될 때 마다 업데이트된 함수를 이벤트 핸들러로 등록하기
  2. 초기에 이벤트 핸들러를 등록하고, State 가 변경 될 때 마다 Ref 에 State 를 저장해 준 후, 이벤트 핸들러에서는 Ref 값을 사용하기

선택한 해결방법

제 생각에는 2번의 방법이 훨씬더 비용상으로 효율적이라고 생각했기 때문에 2번 방법을 선택했습니다. 매번 이벤트를 등록하는 것 보다 Ref 를 수정하는게 효율적이라고 판단했습니다.

3. React.Memo & useCallback 사용 기준

  • Calculator 에 정의된 함수들 중에 어떤 함수에는 useCallback 이 적용되어 있는데요, 저는 일단 , 디펜던시 배열이 비어질 경우, 즉 상태의 변경에도 영향을 받지 않을 함수에만 useCallback 을 사용하여 리렌더링 되어도 함수가 새로 생성되지 않도록 했습니다.
    • 그 이유는 자주 업데이트 되는 state 에 의존이 되는 함수일 경우리렌더링 될 때 함수가 새로 생성되는 비용 < useCallback의 메모이제이션 비용이라고 생각했기 때문입니다. 따라서 현재 어플리케이션 사이즈에서는 오히려 useCallback 으로 계속 메모이제이션 시켜주는게 더 성능상 좋지 않을거라고 판단했어요.
  • 그리고 useCallback 이 적용된 함수가 props 로 전달되는 컴포넌트에 React.memo 를 적용해서 부모컴포넌트가 리렌더링 되어도 자식 컴포넌트는 리렌더링 되지 않도록 했습니다.


hooks로 바꾸면서 조금 과하게 사용한 부분도 없지않아 있는 것 같은데, 여러가지를 학습하는데 중점을 두어 보았습니다 : D
그럼 다시한번 감사드립니다 💯

Copy link

@roy-jung roy-jung left a comment

Choose a reason for hiding this comment

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

호프님 안녕하세요~
리액트 함수형은 또 다르지요?

기존 코드를 최대한 유지하려고 하신 것 같은데, 리액트는 리액트만의 '랜더링 최소화'를 위한 기법들이 많이 존재합니다. 그 기본 철학에 충실히 따른 코드는 아닌 것 같아요. 이에 대해서는 코멘트 남겼으니 확인해보시면 좋겠습니다.

추가로 리뷰요청시에 엄청 긴 글을 작성해주셨는데, 이와 관련하여 몇가지만 덧붙여보겠습니다.

  • 3자리가 넘어 간 후에 숫자를 입력 할 시 마지막 자릿수를 수정해주는 식으로 해결
    : 사용자 입장에서 괜찮은걸까요? 계산기 상단에 문구를 남겨놓으셔서 '3자리수까지만 입력 가능하다'는 사실은 인지가 가능�하지만, 그렇다고 '마지막 자릿수가 변경된다'는걸 인지하기는 어렵고 당황스러울 것 같다는 의견입니다.
  • "출력값 있는 상황에" 페이지 이탈시 이탈 여부를 확인
    : 맞습니다. 다만 항상 그렇게 동작하지는 않는것 같은데, 이부분은 넘어가도 될 것 같네요 :)
  • 버그제보) AC를 눌러 0이 표기된 상태에서 새로고침하면 localStorage에 저장되지 않고, 이탈여부를 확인하지도 않으며, 새로고침 완료시 기존 값이 다시 로드되고 있습니다.

React 체험 성격의 미션이라 바로 승인 및 머지할게요. 위 내용은 참고만 하세요. 고생하셨습니다 :)

const Calculator = () => {
const [state, setState] = useState({
firstOperand: 0,
secondOperand: -1,

Choose a reason for hiding this comment

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

초기값이 -1인 이유는 뭔가요?

Copy link
Author

Choose a reason for hiding this comment

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

이 부분 뒷자리 숫자가 '없다'라는 것을 표현해주기 위해서 일단 -1로 초기화해주었는데요, 0 으로 바꾸는게 더 맞다고 판단됩니다. 지적해주셔서 감사합니다.

operation: null,
isError: false,
});
const handleWindowClose = (e) => {

Choose a reason for hiding this comment

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

이 함수를 어떤 이유에서 이렇게 작성하셨는지는 이해하겠습니다. 다만 함수의 쓰임새가 useEffect 안에서만 이뤄지고 있으니, useEffect 안으로 옮기시는 편이 더 나을 것 같아요. 현재 상태로는 Calculator가 render될 때마다 새롭게 정의되는 거거든요.

Copy link
Author

Choose a reason for hiding this comment

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

넵! 일단 함수를 다 분리한다는 목적으로 했는데, 렌더될때마다 함수가 새로 생성되어서 성능상 좋지 못하다는 것은 간과했네요 😭 지적해주셔서 감사합니다.

secondOperand: -1,
operation: null,
isError: false,
});
Copy link

@roy-jung roy-jung Apr 26, 2022

Choose a reason for hiding this comment

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

하나의 state에 모든 정보를 객체형태로 넣는 것은 안티패턴입니다.
이상태로는 어떤 값 하나만 변경되더라도 변경되지 않은 다른 모든 하위컴포넌트가 다시 렌더될거에요.
변경된 부분만 렌더링하자! 라는 리액트의 컨셉에 정면으로 배치되는 셈이죠.

Copy link
Author

Choose a reason for hiding this comment

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

저는 일단 계산기 내에서 각각 상태값이 거의 함께 변경되어서 객체형태로 수정해주었는데요,
로이님 말씀을 듣고보니까 불필요한 하위 컴포넌트의 리렌더링을 일으킬 수 있겠군요! 지적해주셔서 감사합니다 :)

@roy-jung roy-jung merged commit a85b207 into woowacourse:moonheekim0118 Apr 26, 2022
@moonheekim0118
Copy link
Author

안녕하세요 로이님, 간단한 미션이었는데도 좋은 리뷰 주셔서 정말 감사드려요!
말씀주신 기능적인 문제나, 리액트적인 문제들 모두 잘 고민해보고 지식으로 녹여내보도록 하겠습니다 :D 👍 👍
정말 감사드립니다!

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.

2 participants