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

Life cycle hook 개발 #3

Merged
merged 12 commits into from
Oct 31, 2022
Merged

Life cycle hook 개발 #3

merged 12 commits into from
Oct 31, 2022

Conversation

hyesungoh
Copy link
Member

@hyesungoh hyesungoh commented Oct 16, 2022

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

  • useEffect만으로 life cycle을 접근, 제어하는 것보다 가독성이나 성능적으로 이점이 있다고 생각되어,
    저는 이렇게 쓰곤 해요. 여러분의 의견이 궁금해용

🎉 어떻게 해결했나요?

  • useDidMount, useDidUpdate, useWillUnmount라는 이름으로 개발 및 테스트 코드를 간단히 적어 봤어요.

  • useDidMount에서 default export를 하고, 같은 dir의 index에서 default export를 하기 위해
    no-restricted-exports rule을 해제했어요

    이렇게 파일 디렉토리를 가져간 이유는
    사용하는 곳에서는 폴더명으로 import하며

    import useDidMount from 'hooks/useDidMount'

    개발하는 ide에서 폴더명을 직관적으로 보기 위함이라 이렇게 하는 것을 좋아하곤 합니당

    스크린샷 2022-10-16 오후 5 54 09
  • no-unused-vars rule을 활성화 했어요. 대신 _는 사용하지 않아도 되도록 했어요

  • test:watch script를 추가했어요

📚 Attachment (Option)

  • 일단 파일 명을 일반적인 카멜 케이스 (useDidMount)로 적어 봤는데,
    hook의 경우 케밥 케이스(use-did-mount)도 읽기 편하더라구요.
    선호하시는 게 있나 궁금해요

  • 그리고 맞춤법이나.. 영어 작문이 이상한 부분이 있다면 ... 거침없이 혼내주세요

@hyesungoh hyesungoh self-assigned this Oct 16, 2022
@codecov-commenter
Copy link

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (5ce19fe) compared to base (c1a7ed4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #3   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         7    +6     
  Lines           21        46   +25     
  Branches         6         8    +2     
=========================================
+ Hits            21        46   +25     
Impacted Files Coverage Δ
src/hooks/useDidMount/index.ts 100.00% <100.00%> (ø)
src/hooks/useDidMount/useDidMount.ts 100.00% <100.00%> (ø)
src/hooks/useDidUpdate/index.ts 100.00% <100.00%> (ø)
src/hooks/useDidUpdate/useDidUpdate.ts 100.00% <100.00%> (ø)
src/hooks/useWillUnmount/index.ts 100.00% <100.00%> (ø)
src/hooks/useWillUnmount/useWillUnmount.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

Bundle Sizes

Compared against c1a7ed4

Route: No significant changes found

Dynamic import: None found.

@hyesungoh hyesungoh added feat New feature test: unit generate or update unit test labels Oct 16, 2022
Copy link
Collaborator

@kooku0 kooku0 left a comment

Choose a reason for hiding this comment

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

갠적으로 index 별로 안좋아하긴합니다 ㅋㅋ
불필요한 파일들도 만들어지고, 변경에 대해 더 많은 파일을 수정해야해서 ㅎㅎ

Comment on lines 4 to 10
const didMountRef = useRef<boolean>(false);

useEffect(() => {
if (!didMountRef.current) {
didMountRef.current = true;
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

오오 이건 설마.. react18 대응인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

18을 따로 대응할 생각은 없었어요 ㅋㅋㅋㅋ

아마 react18 dev에서 effect가 두 번 호출되는 사항을 말씀하신... 거죠..?

단순히 mount 시에는 실행되지 않게 하고, 디펜던시가 업데이트된 후에 실행되도록 작성하기 위한 부분이에요

논외지만 지금 생각해보니 did mount, did update에서 EffectCallback 타입이 아니라 VoidFunction 타입이 더 적절할 것 같네요 ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

네 맞아요. effect 두번 호출되는거.. ㅠ

Comment on lines 19 to 43
const STATE_CHANGE_BUTTON_TEXT = 'change';
const mockCallback = jest.fn();

const App = () => {
const [state, setState] = useState<number>(0);

useDidUpdate(mockCallback, [state]);

return (
<div>
<button type="button" onClick={() => setState((prev) => prev + 1)}>
{STATE_CHANGE_BUTTON_TEXT}
</button>
</div>
);
};

afterEach(() => {
jest.clearAllMocks();
});

it('마운트 시 effectCallback이 실행되면 안된다', () => {
render(<App />);
expect(mockCallback).not.toBeCalled();
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

뭔가.. 간단하게 만들 수 있을 것 같은뎅..

import { renderHook } from '@testing-library/react-hooks';

import useDidMount from './useDidMount';

describe('useDidMount', () => {
  const effectCallback = jest.fn();

  beforeEach(() => {
    effectCallback.mockClear();
  });

  it('default export이여야 한다', () => {
    expect(useDidMount).toBeDefined();
  });

  it('effectCallback이 실행되어야 한다', () => {
    renderHook(() => useDidMount(effectCallback));
    expect(effectCallback).toBeCalled();
  });

  it('rerender 시 1번 실행되어야 한다', () => {
    const { rerender } = renderHook(() => useDidMount(effectCallback));
    rerender();
    expect(effectCallback).toBeCalledTimes(1);
  });

  it('mockCallback이 실행되어야 한다', () => {
    renderHook(() => useDidMount(effectCallback));

    expect(effectCallback).toBeCalledTimes(1);
  });

  it('mockCallback은 상태가 변해도 1번 실행되어야 한다', () => {
    const { rerender } = renderHook(() => useDidMount(effectCallback));

    rerender();

    expect(effectCallback).toBeCalledTimes(1);
  });
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 위에 똑같은 테스트코드가 있었네요 ㅋㅋ

@hyesungoh
Copy link
Member Author

갠적으로 index 별로 안좋아하긴합니다 ㅋㅋ 불필요한 파일들도 만들어지고, 변경에 대해 더 많은 파일을 수정해야해서 ㅎㅎ

맞아요 ㅋㅋㅋㅋ 지금만 해도 index의 테스트 파일들도 있어서 ㅋㅋㅋㅠ

├── hooks
│   ├── useDidMount
│   │   ├── useDidMount.test.tsx
│   │   └── useDidMount.ts

그럼 평소에 이렇게 가져가시나요?

├── hooks
│   ├── api
│   ├── foobar
│   ├── common
│   │   ├── useDidMount.test.tsx
│   │   └── useDidMount.ts

이렇게도 가져갈 수도 있을 거 같고 .. 아니면 한 depth 더 들어갈 수도 잇을 거 같구요

급한 건 아니라, 수요일이나 토요일 회의 시간에 의견 나눠봐도 좋을 거 같슴다 ~

의견 감사드려요 👍 👍

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 19, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3dae954
Status: ✅  Deploy successful!
Preview URL: https://8365aa7a.12-team3-web.pages.dev
Branch Preview URL: https://feat-life-cycle-hooks.12-team3-web.pages.dev

View logs

@hansol-FE
Copy link
Collaborator

hansol-FE commented Oct 22, 2022

useEffect가 모두 가지고 있는 기능이고 이걸 세분화해서 가독성을 높인건 알겠는데 성능적인 이점은 뭐가 있는지 궁금하네요... 저 아직 리액트 초보입니다.ㅠㅠ

@hyesungoh
Copy link
Member Author

hyesungoh commented Oct 22, 2022

useEffect가 모두 가지고 있는 기능이고 이걸 세분화해서 가독성을 높인건 알겠는데 성능적인 이점은 뭐가 있는지 궁금하네요... 저 아직 리액트 초보입니다.ㅠㅠ

말씀하신대로 가독성이 중점적인 이유입니다!

제가 생각하기에 성능적인 장점이라면, useEffect를 어떻게 사용하냐에 따라 있을 수도 있고 없을 수도 있다고 생각되는데요.

useEffect(() => {
  마운트때만해야되는일();
  
  업데이트시에만해야되는일();

  return 언마운트시에해야되는일();
}, [디펜던시]);

이처럼 한 effect에 작성하게 되면, 불필요하게 실행되는 코드들이 있지만

useDidMount(() => {
  마운트때만해야되는일();
});

useDidUpdate(() => {
  업데이트시에만해야되는일();
}, [디펜던시]);

useWillUnmount(() => {
  언마운트시에해야되는일();
}); 

이렇게 분리하게 되면 불필요하게 실행되는 코드들을 방지할 수 었어 성능적인 이점이 생긴다고 생각합니다.

useEffect(() => {
  마운트때만해야되는일();
});

useEffect(() => {
  // did mount시 실행 됨
  업데이트시에만해야되는일();
}, [디펜던시]);

useEffect(() => {
  return 언마운트시에해야되는일();
}); 

물론 이렇게 useEffect만을 이용해서도 분리할 수 있지만, 말씀하신 가독성을 이유로 저는 유용하게 쓰곤 합니다!

그렇다고 꼭 사용해야되는 것은 아니라고 생각해요.
같은 관심사를 갖으며 여러 life cycle에 접근하는 경우에는 useEffect를 쓰는 형태랄까요!

@eunddodi
Copy link
Collaborator

useEffect 내에서 상황에 따라 분기하는 게 다소 성가시다고 느꼈었어서 저는 라이프사이클 훅 써보면 좋을 거 같아유 무지성으로 useEffect에 집어넣는 대신 코드 짤 때 한 번 더 생각할 수 있을 거 같고요

Copy link
Collaborator

@kooku0 kooku0 left a comment

Choose a reason for hiding this comment

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

👍🏼

@hyesungoh hyesungoh merged commit cffdd6d into main Oct 31, 2022
@hyesungoh hyesungoh deleted the feat/life-cycle-hooks branch October 31, 2022 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature test: unit generate or update unit test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants