-
Notifications
You must be signed in to change notification settings - Fork 2
[UNI-195] Post 요청에 대한 debounce 적용 #110
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 어려운 로직 구현하느라 고생하셨고 실수가 있었지만 이 과정에서 백엔드도 에러처리 로직도 신경쓰고 저와 tanstack-query 등 많은 조사를 통해 배워가는게 더 많았던 경험이었습니다!
mutate 2번만 수정하시면 approve 하겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
되게 어려운 로직임에도 매우 훌륭하게 수행해주셔서 감사합니다!
|
||
export function useDebounceMutation<TData, TError, TVariables, TContext>( | ||
options: UseMutationOptions<TData, TError, TVariables, TContext>, | ||
delay: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delay같은 경우는 default값을 부여하는것이 좋을 것 같습니다!
@@ -25,45 +26,49 @@ export default function useMutationError<TData, TError, TVariables, TContext>( | |||
handleError?: HandleError, | |||
): UseMutationErrorReturn<TData, TError, TVariables, TContext> { | |||
const [isOpen, setOpen] = useState<boolean>(false); | |||
const result = useMutation<TData, TError, TVariables, TContext>(options, queryClient); | |||
const result = useDebounceMutation<TData, TError, TVariables, TContext>(options, 1000, true, queryClient); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
커스텀 훅을 생성해두길 잘했다고 생각이 드는 부분입니다....
덕분에 유지보수 중 각 컴포넌트에서는 신경을 쓰지 않아도 된다는 점이 좋슴니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런것까지 보셨던건가요?? 만들어놓길 잘한 것 같습니다
endNodeId: "nodeId" in lastPoint ? lastPoint.nodeId : null, | ||
coordinates: subNodes, | ||
}); | ||
mutate({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 과정에서 사용된 mutate가 2번 들어갔습니다.
debounce로 인해 일반적인 겅우 무시가 될 수 있겠지만 에러가 발생할 여지를 안만드는게 좋을 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다~수정했습니다
<Button onClick={reportNewRoute}>제보하기</Button> | ||
<Button | ||
onClick={reportNewRoute} | ||
variant={status === "pending" || status === "success" ? "disabled" : "primary"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
status !== "error"
라고도 할 수 있을 것 같지만. 진행 중 혹은 성공 완료라는 의미가 한 번에 와닿지 않을 수 있어서 취향에 맞게 적용하셔도 될 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 state !== "idle"도 적용을 해야해서, 더 명시적인 코드로 작성했는데, 피드백 감사합니다!
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 고생하셨습니다!
#️⃣ 작업 내용
장애 리포트(백엔드 죄송합니다..)
문제 상황
제작한 debounce를 검증하기 위해 mock과 console을 활용해 테스트하다가, 길 생성 API의 실제 테스트를 하기 위해서 실제 API로 따닥 테스트를 했습니다.
그 결과, 백엔드의 길 조회 API에 장애를 일으켜 잠깐 백엔드 개발 서버의 응답이 느려지는 결과가 있었습니다. 알고보니, debounce를 테스트하고 있었던 개발 환경이 아니라, debounce가 배포되지 않은 배포 사이트에서 테스트하고 있었습니다.
해결 방법
따닥 테스트로 추가된 똑같은 두가지 길을 제거하고, 서버를 재배포해 해결되었습니다.
깨달은 점
멘토님과 팀원들과 이야기하며, 현업에서는 절대 이런 식으로 테스트 안된다는 것을 깨달았고, 다음에는 꼭 백엔드와 협의하고, 안전한 상황에서 테스트를 할 수 있도록 하려고 합니다.
또한, 테스트 코드도 이런 부분들은 꼭 작성해야 한다는 것을 깨달았고, 개발환경에서 테스트를 하고 있는지도 꼭 확인해보겠습니다.
이 기능을 제작한 이유
백엔드가 QA에서 말씀해주시기도 해서 도입한 이유도 있지만, 저는 이전 프로젝트에서 같은 요청을 빠른 따닥(?)으로 중복 요청해 게시물이 여러개가 올라가는 경험을 했었습니다. 그럴 때마다 해결할 때 라이브러리를 사용하거나,loading(pending) 상태일 때 버튼을 비활성화하는 방법을 사용했습니다.
하지만 state를 변경하면서 loading이나 pending 상태로 전환되는 그 찰나에 작동하는 따딱이 있었고, 그런 부분들을 보완하지 못해 항상 아쉬웠습니다. 이런 이유로 이번 프로젝트를 통해 꼭 그런 동작을 막아보고 싶었습니다.
주요 기능
debounce 코드를 제작했습니다.
기초가 된 debounce 코드는 다음과 같이 제작했습니다.
동작 방식(버튼 클릭 기준)
버튼을 클릭할 때의 조건은, time = 1000, 즉시 실행되어야하기 때문에 isImmediate를 true로 놓는 시나리오입니다.
-> later 함수를 선언 받고, callNow가 true가 됩니다(timeout이 선언된 것이 없고, immediate는 true이기 때문에)
-> timeout이 없기 때문에, clearTimeout을 넘어간 이후, later를 callback으로 넣은 새로운 timeout을 만들어줍니다.
-> callNow가 true였기 때문에, 넣었던 함수를 인자들과 함께 실행해줍니다.
-> later 함수를 다시 선언받고, callNow는 false가 됩니다.
-> timeout을 클리어 해준 이후, 다시 later의 Timeout을 선언해줍니다.
-> later의 시간이 이미 지난 시간 + time 만큼 지연되게 됩니다.
=> 결국 func는 처음 한번만 실행되게 되고, time 시간안에 debounce에 들어오게 되면 이미 실행되고 있는 timeout closure가 있기 때문에 아무 동작도 못하게 됩니다. (만약 실행을 지연시켜야 한다면, isImmediate = false를 통해 later가 debounce를 그만 실행할 때까지 지연이 가능합니다.)
이런 동작으로 인해, 만약 state가 바뀌는데 걸리는 시간이 50ms정도 걸린다면(혹은 컴포넌트가 무거워 그 이상의 시간이 걸린다면) debounce로 원하는 동작을 실행후 다시 실행을 막거나, 그만할 때까지 막는 동작을 할 수 있습니다.
이 방식을 useMutation과 결합해서 사용 방법을 찾아보았습니다.
맨 처음에는 mutate를 실행하는 함수 자체를 debounce로 감싸는 시도를 해보았습니다.
const reportRoute = debounce(()=>{}, 1000, true);
이런 시도가 잘 되었지만, reportRoute를 실행할 때 리렌더링이 발생해 매번 다른 timeout을 바라보는 문제가 생겼고, useCallback 사용하는 변수들로 감싸주어 참조를 통제해야하는 현상이 발생했습니다. (closure를 활용해야하기 때문에).
useCallback을 사용하면서 두가지 사고의 흐름이 다음과 같았습니다.
그래서 나온 결과물이 다음과 같습니다.
useRef가 HTMLElement를 저장하는 것뿐만 아니라, 이런 식으로 값이나 참조를 유지할 수 있는 용도로도 사용이 가능하다고 해서, 적용해보았습니다.
형태는 유사하지만, react의 동작에 맞춰 여러가지 hook을 사용했습니다.
이 hook을 기존에 useMutationError hook가 이런 방식으로 결합했습니다.
이후 백엔드 팀원들과 동현님과 정상동작을 N번정도 확인한 이후에,, PR을 올리게 되었습니다 감사합니다 팀원 여러분..!!!
동작확인
_talkv_wxat6WM0Mt_MyKKVgJQN0HIniQpTSQZOk_talkv_high.MP4