feat(react): useBlockPromiseMultipleClick 추가 #82
Conversation
|
There was a problem hiding this comment.
@Sangminnn 우선 기여를 위해 PR을 생성해주셔서 감사드립니다! 너무나도 유용한 훅인 것 같습니다...!
개인적으로 아래와 같은 추가 의견이 있는데 검토해주시면 감사드립니다!
beforeEach(() => {
vi.useFakeTimers();
});
afterEach(() => {
vi.useRealTimers();
});
describe('useBlockDoubleClick', () => {
it('should block double click', async () => {
const mockFn = vi.fn(async () => await delay(1000)); // (*) mock 함수를 활용
const { result } = renderHook(useBlockDoubleClick);
const { blockDoubleClick } = result.current;
expect(result.current.isLoading).toBe(false); // (*) 아래 isLoading을 체크해주는 부분과 같이 통일해주는 것도 좋아보입니다!
const onClick = () => blockDoubleClick(mockFn);
const { user } = renderSetup(
<button onClick={onClick}>TestButton</button>,
{ delay: null } // timeout issue 해결
);
const button = screen.getByRole('button'); // (*) getByRole 사용
await user.click(button);
await user.click(button);
expect(result.current.isLoading).toBe(true);
expect(mockFn).toBeCalledTimes(1);
await vi.advanceTimersByTimeAsync(1000); // timer 추가
expect(result.current.isLoading).toBe(false); // (*) isLoading false 변경 케이스 추가
});
});-
위와 같이
isLoading이 false로 변경되는 케이스도 추가하는 것은 어떨까요..!? 🤔 -
추가적으로 blockDoubleClick으로 넘겨주는 함수가 몇 번 호출되었는지가 테스트 주요 포인트이기 때문에
vi.fn을 활용해서mockFn(목업 함수)를 생성하고toBeCalledTimes와 함께 활용하면 좋을 것 같습니다!- counter와 같은 변수도 불 필요해 질 것 같습니다 🙇♂️
-
testing library/react 를 v15로 올렸다가 userEvent와 fakeTimer 사용 시에 timeout 이슈가 발생해서 아래 PR을 통해 다시 v14로 버전을 낮췄습니다.
revert: @testing-library/react 버전 v15 -> v14 revert #83
|
|
||
| const { user } = renderSetup(<button onClick={onClick}>TestButton</button>); | ||
|
|
||
| const button = screen.getByText('TestButton'); |
There was a problem hiding this comment.
| const button = screen.getByText('TestButton'); | |
| const button = screen.getByRole('button'); |
getByRole로 요소를 가져오는게 더 좋아보이는데 어떨까요!? 아래 testing-library 문서를 통해 우선순위를 가이드하는데 getByRole의 우선순위가 높기 때문입니다! 🙏
| const useBlockDoubleClick = () => { | ||
| const [isLoading, setIsLoading] = useState(false); | ||
| const isClicked = useRef(false); | ||
|
|
||
| const blockDoubleClick = async (callback: () => Promise<unknown>) => { | ||
| if (isClicked.current) { | ||
| return; | ||
| } | ||
|
|
||
| isClicked.current = true; | ||
| setIsLoading(true); | ||
|
|
||
| await callback(); | ||
|
|
||
| isClicked.current = false; | ||
| setIsLoading(false); | ||
| }; | ||
|
|
||
| return { | ||
| isLoading, | ||
| blockDoubleClick, | ||
| }; | ||
| }; | ||
|
|
||
| export default useBlockDoubleClick; |
There was a problem hiding this comment.
export const useBlockDoubleClick = () => {}default exports와 named exports를 혼합해서 사용하는 것을 권장하지 않기때문에 다른 모듈들과 컨벤션을 지키기 위해 useBlockDoubleClick을 default export말고 named export로 변경해주시면 감사드립니다!
|
@Sangminnn 네이밍에 대한 고민도 있는데요! 사실 해당 훅은 더블 클릭 뿐만 아니라 3번, 4번그 이상도 막아주는 것으로 이해했는데요! 또한 Promise를 block하는 것이기 떄문에 |
Sangminnn
force-pushed
the
feat/useBlockDoubleClick
branch
from
7553937
to
cde6353
Compare
|
@ssi02014 좋은 의견 감사합니다! 전반적으로 말씀주신 의견과 가이드를 반영하였고, hook의 네이밍만 use(동사)(명사) 의 형태로 작성하는 것이 더 자연스러워보여 이와 같이 네이밍을 변경해두었습니다! |
|
@Sangminnn 수정 감사합니다!! 앗 마지막으로 훅 내부 blockDoubleClick 함수도 네이밍을 수정이 되면 좋을 것 같습니다!! |
|
@ssi02014 감사합니다! 수정과정에서 해당 메서드의 네이밍 변경을 놓쳤네요! 맥락에 맞게 blockMultipleClick으로 메서드명을 수정해두었습니다 :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #82 +/- ##
==========================================
+ Coverage 82.05% 82.44% +0.39%
==========================================
Files 63 64 +1
Lines 535 547 +12
Branches 114 115 +1
==========================================
+ Hits 439 451 +12
Misses 84 84
Partials 12 12
|
This reverts commit 941af5d.
ssi02014
changed the title
|
|
Overview
useDebounce는 동일한 이벤트가 중복으로 발생할 경우 이를 적게 호출할 수 있도록 도와줍니다. 또한 이러한 특징을 활용하여 순간적인 중복호출을 방지하는 역할을 하기도 하는데, 이는 시간을 다루는 작업이기에 완벽하게 막아낼 수 없습니다.
이 훅은 특정 Promise동작을 수행하는 동안은 중복호출이 불가능하도록 만들어둔 훅입니다.
양식에 맞지않거나 의견주실 부분이 있다면 편하게 말씀해주시면 감사드리겠습니다 :)
PR Checklist
Contributing Guide