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

[SP1] 활동후기 페이지 요청 많이 가는 에러 수정 #191

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/hooks/useStackedFetchBase/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { to } from 'await-to-js';
import { useEffect, useReducer } from 'react';
import { debounce } from '@src/utils/debounce';
import { reducer } from './reducer';
import { Action, State } from './types';

Expand All @@ -14,7 +13,7 @@ function useStackedFetchBase<T>(
});

useEffect(() => {
const fetchList = debounce(async () => {
const fetchList = async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

debounce 는 사용하지 않아서 지웟습니다 ..!!

dispatch({
_TAG: 'FETCH',
isInitialFetching,
Expand All @@ -32,7 +31,7 @@ function useStackedFetchBase<T>(
if (response) {
dispatch({ _TAG: 'SUCCESS', isInitialFetching, data: response });
}
});
};

fetchList();
}, [willFetch, isInitialFetching]);
Expand Down
18 changes: 16 additions & 2 deletions src/views/ReviewPage/hooks/useFetch.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,31 @@
import { useCallback, useEffect } from 'react';
import { useCallback, useEffect, useRef } from 'react';
import useBooleanState from '@src/hooks/useBooleanState';
import useStackedFetchBase from '@src/hooks/useStackedFetchBase';
import { api } from '@src/lib/api';
import { ExtraPart } from '@src/lib/types/universal';
import useInfiniteScroll from './useInfiniteScroll';

const useFetch = (selected: ExtraPart) => {
const { ref, count, setCount } = useInfiniteScroll();
const jobRecordRef = useRef<Record<number, 'QUEUED' | 'DONE'>>({});
const isIncrementable = () =>
Object.values(jobRecordRef.current).every((value) => value === 'DONE');

const { ref, count, setCount } = useInfiniteScroll(isIncrementable);
const [canGetMoreReviews, setCanGetMoreReviews, setCanNotGetMoreReviews] = useBooleanState(true);

useEffect(() => {
jobRecordRef.current[count] = 'QUEUED';
}, [count]);

Copy link
Member Author

Choose a reason for hiding this comment

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

이걸 willFetch 함수 안에서 하려 그랬는데,
useCallback은 의존성 배열에 뭐가 담겨 있어도 빠릿빠릿하게 호출되진 않더라구요..

useEffect(() => {
function initializeStates() {
Copy link
Contributor

Choose a reason for hiding this comment

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

question;

image

-> useEffect에서 의존성으로 setCanGetMoreReviews, setCount 을 넣어주는 이유는 무엇인가요?
두 값은 변하지 않는다고 이해해가지구 selected가 변경될때만 useEffect가 실행되면 되는거 아닌가? 라고 생각이 들었어요!

Copy link
Member

Choose a reason for hiding this comment

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

setCanGetMoreReviews, setCount 가 처음 컴포넌트 읽는 순간부터 unmount 시점까지 안변하나요??

Copy link
Contributor

Choose a reason for hiding this comment

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

setCanGetMoreReviews가 useBooleanState에서 const setTrue = useCallback(() => { setBool(true); }, []); 이고
setCount는 useInfiniteScroll에서 const [count, setCount] = useState(1);인데
둘다 변수가 아니라고 생각해서 처음 마운트때 이후로 바뀌지 않는다고 이해했어요!

잘못 이해한거라면 알려주세요...!!

Copy link
Member

@joohaem joohaem Sep 21, 2023

Choose a reason for hiding this comment

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

오오 생각 정리 좋아요!

이게 이전에는 set~ 함수 또한 변수고, 렌더링 때와 한번 마운트 되고의 값이 다르니 react-hooks/exhaustive-deps 같은 린트에 "set~도 넣어줘!" 라며 warning을 띄워줬는데, 요새에는 사람들도 "stable 한 값인데 이거 넣어야 하냐" 하고 안넣는 추세인 듯 해요!
ref: reactjs/react.dev#4506

Copy link
Contributor

Choose a reason for hiding this comment

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

아하 그렇게 setState 변수로 보아서 넣는 경우도 있군요!
감사합니다!! 👍

setCount(1);
setCanGetMoreReviews();
for (const key in jobRecordRef.current) {
if (Object.hasOwnProperty.call(jobRecordRef.current, key)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

페이지가 바뀌면 (예시: 전체 선택했다가 기획 선택하면) jobRecordRef.current 를 초기화해줍니다 ({}로 만들어줍니다)

delete jobRecordRef.current[key];
}
}
}
Comment on lines +24 to +28
Copy link
Member

Choose a reason for hiding this comment

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

반복 돌지 않고 joRecordRef.current = {} 때려주는 게 "initialize"에 좀 더 적합하지 않나 싶어요 !!
그렇게 않은 이유가 있을까요???

Copy link
Member

Choose a reason for hiding this comment

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

근간은 아니지만,, delete 키워드 찾다가 생각하게 된 것입니다😇
https://javascript.plainenglish.io/avoid-the-delete-keyword-in-javascript-87ff2a47f26c

return () => {
initializeStates();
Expand All @@ -23,6 +36,7 @@ const useFetch = (selected: ExtraPart) => {
setCanNotGetMoreReviews();
const response = await api.reviewAPI.getReviews(selected, count);
Copy link
Contributor

Choose a reason for hiding this comment

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

question;

willFetch가 실행될때 setCanNotGetMoreReviews();를 실행하고 api 요청을 하는 이유가 무엇인가요?

Copy link
Member

Choose a reason for hiding this comment

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

@f0rever0
canGetMoreReviews 가 어떤 변수인가요?

Copy link
Contributor

Choose a reason for hiding this comment

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

useBooleanState 에서 canGetMoreReviews을 false로 만들어 주는 함수인데,
willFetch에서 canGetMoreReviews를 false시키고 response.hasNextPage ? setCanGetMoreReviews() : setCanNotGetMoreReviews();로 api 요청을 통해 다시 값을 재정의 해주는 이유가 궁금했어요~

  const willFetch = useCallback(async () => {
    setCanNotGetMoreReviews();
    const response = await api.reviewAPI.getReviews(selected, count);
    response.hasNextPage ? setCanGetMoreReviews() : setCanNotGetMoreReviews();
    jobRecordRef.current[count] = 'DONE';
    return response.reviews;
  }, [selected, count, setCanGetMoreReviews, setCanNotGetMoreReviews]);

response.hasNextPage ? setCanGetMoreReviews() : setCanNotGetMoreReviews();
jobRecordRef.current[count] = 'DONE';
return response.reviews;
}, [selected, count, setCanGetMoreReviews, setCanNotGetMoreReviews]);
const state = useStackedFetchBase(willFetch, count === 1);
Expand Down
6 changes: 4 additions & 2 deletions src/views/ReviewPage/hooks/useInfiniteScroll.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { useState } from 'react';
import useIntersectionObserver from '@src/hooks/useIntersectionObserver';

export default function useInfiniteScroll() {
export default function useInfiniteScroll(isIncrementable: () => boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

@SeojinSeojin
좋은 인터페이스가 될지에는 의문이 있어요..! '정말 필요한 게 맞나?', '그냥 하나의 엣지케이스를 위해 뚫은 인터페이스이지 않나?' 하는 노파심이랄까요😇😇


그리고 () => boolean 타입인데 is~ 의 이름을 가지는 이유가 무엇일까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

observer box 를 없애는 방식에 썼던 변수가 제대로 위의 문제로 제대로 업데이트되지 않는 상황이었습니다 ..!!

Copy link
Member Author

Choose a reason for hiding this comment

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

좋은 인터페이스가 될지에는 의문이 있어요..! ..

맞아요 ....... [스크롤 이벤트]랑 [서버 요청]이랑 알잘딱 독립적으로 진행되도록 수정하는 것이 베스트일 것 같습니다 ..!!!

그러나 요청이 다 제대로 왔는지에 대한 상태를 확인하는 변수로부터 현재 스크롤 가능한지 확인하는 방법도 괜찮을 것 같다고 생각했습니다!
1,2,3,4,...페이지의 요청 완료 여부를 한 변수에서 확인하는 데에는 어려움이 있어서요..!!

그럼 isIncrementable 을 훅에서 리턴해서, 컴포넌트에서 쓰도록 하는것이 좋을까요 ..?
그러나 결국 저것을 위해 사용될 텐데 그래도 자율성을 주는 것이 좋을까요 ..??

Copy link
Member Author

Choose a reason for hiding this comment

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

그리고 () => boolean 타입인데 is~ 의 이름을 가지는 이유가 무엇일까요?

거의 변수처럼 쓰이는 함수이기 때문이었습니당 헤헤...
좋은 이름으로 고치겠습니다!!

Copy link
Contributor

Choose a reason for hiding this comment

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

@joohaem 좋은 인터페이스인가를 염려한 이유가 스크롤 이벤트에서 서버에서의 요청 상태를 isIncrementable로 확인하는 부분이 서진이 말처럼 [스크롤 이벤트]랑 [서버 요청]이랑 독립적으로 진행되지 않는다 라서 그런걸까요!?
이해한게 맞는지 궁금해서요~

Copy link
Member

@joohaem joohaem Sep 21, 2023

Choose a reason for hiding this comment

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

@f0rever0
A 라서 B 였던 건 아니고, C를 고려하다가 B를 생각해서 서진이가 A라는 해법을 생각한 맥락인 것 같네요?!

C는 무엇이냐 하면, 훅의 인자나 interface나 마찬가지로 "개발"을 위해 뚫는 경우가 많아요. 이건 그 훅이나 객체가 가지는 정체성을 흐리는 행위고, 이렇게 뚫는 인터페이스가 많아질 수록 의존성이 엉키게 되면서 스파게티 코드가 될 가능성이 높아진다고 생각해요
isIncrementable이라는 인자값이 useInfiniteScroll 이라는 훅의 인자값이라고 기대가 되나요? 전 잘 안돼요. 왜냐하면 다른 부분들의 useInfiniteScroll 들에서는 인자값 없이 사용해왔고, 그 자체의 성격을 가지고 있으니까요. 인자값이 추가되어 거기에 의존한다면, 정말 차라리 useInfiniteScroll 이라는 훅의 네이밍이 바뀌어야 하지 않나 싶어요. 혹은 인자값을 훅의 사용자 및 내부가 기대할 수 있게끔 (could~ 이라던가?) 뚫는다거나, 서진이 해법처럼 테마에 따라 나누는 것이 깔끔할 수 있고요.

Copy link
Contributor

Choose a reason for hiding this comment

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

@joohaem 음 그렇군요!! 더 나은 해법이 있는지 고민해봐야겠네요. 설명 감사합니다 !!! ☺️

Copy link
Member

@joohaem joohaem Sep 21, 2023

Choose a reason for hiding this comment

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

@SeojinSeojin

그럼 isIncrementable 을 훅에서 리턴해서, 컴포넌트에서 쓰도록 하는것이 좋을까요 ..?
그러나 결국 저것을 위해 사용될 텐데 그래도 자율성을 주는 것이 좋을까요 ..??

얕게나마 생각해보았지만 역시나만시나 서진이가 짠 방향보다 나은 것은 생각나지 않습니다...
컴포넌트까지 가기는 아닌 것 같고,, 훅 내에서 이렇게 처리하는 게 맞는 정도인 것 같네요!

const [count, setCount] = useState(1);

const ref = useIntersectionObserver(
async (entry, observer) => {
setCount((prevCount) => prevCount + 1);
if (isIncrementable()) {
setCount((prevCount) => prevCount + 1);
}
observer.unobserve(entry.target);
},
{ rootMargin: '80px' },
Expand Down
Loading