[권수형] sprint7

[Ospac]

https://awkward-panda-market.netlify.app/items/634

요구사항

[기본]

상품 상세

• 상품 상세 페이지 주소는 "/items/{productId}" 입니다.

• response 로 받은 아래의 데이터로 화면을 구현합니다.
  => favoriteCount : 하트 개수
  => images : 상품 이미지
  => tags : 상품태그
  => name : 상품 이름
  => description : 상품 설명

• 목록으로 돌아가기 버튼을 클릭하면 중고마켓 페이지 주소인 "/items" 으로 이동합니다

• 상품 문의 댓글

• 문의하기에 내용을 입력하면 등록 버튼의 색상은 "3692FF"로 변합니다.

• response 로 받은 아래의 데이터로 화면을 구현합니다
  => image : 작성자 이미지
  => nickname : 작성자 닉네임
  => content : 작성자가 남긴 문구
  => description : 상품 설명
  => updatedAt : 문의글 마지막 업데이트 시간

[심화]

• 모든 버튼에 자유롭게 Hover효과를 적용하세요.

주요 변경사항

• CSS 라이브러리를 styled-component에서 scss로 변경했습니다. (때문에 변경사항이 무척 많습니다 😅😅)
  • 기존의 문제점: styled-components에서 design token을 위해 global theme까지 사용하니 스타일링을 위한 코드가 너무 길어지고, 중복되는 스타일 코드가 생김
  • 향상된 지점: module.scss 사용으로 파일단위로 css를 모듈화 + 순수 css의 class 사용으로 파일 내부에서 스타일 상속 및 일관성 유지
• 폴더구조 변경
  • Pages 폴더 내부를 좀더 세분화
  • 개별 도메인에 사용되는 api 파일들의 위치를 전역에서 지역으로 변경 (src/apis -> pages/페이지이름/lib)
• AddItem 페이지의 state를 useReducer로 관리하도록 변경했습니다.
• useAsync -> useFetch / useQuery, useMutation ( tanstack query와 비슷하지만 캐싱 기능이 없는 형태로 모사해보기)
  • useAsync로 fetch 에러처리, loading, error state를 관리하는 형태에서 세분화하여
  • 데이터 페칭을 위한 useQuery, 데이터 수정 및 삭제를 위한 useMutation으로 분리하고 fetch까지 내부의 useEffect에서 처리

멘토에게

1번 질문

export default function useFetch({
  asyncFunction,
  deps = [],
  immediate = false,
}) {
  const [state, setState] = useState({
    data: null,
    loading: false,
    error: null,
  });
  const refetch = useCallback(async () => {
    setState((prev) => ({ ...prev, error: null, loading: true }));
    try {
      const response = await asyncFunction();
      setState((prev) => ({ ...prev, loading: false, data: response }));
    } catch (error) {
      setState({ data: null, loading: false, error });
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [...deps]);

useFetch 함수를 보면 refetch의 의존성 배열을 deps 매개변수로 관리하고 있습니다.
여기서 asyncFunction을 빼면 lint 경고가 뜨는데 무시해도 되는 걸까요?

  const { loading, error, data } = useQuery({
    queryFn: () => getProducts({ ... }),
    deps: [pageSize],
  });

asyncFunction은 useQuery에서 queryFn에 해당합니다. 이 부분을 의존성 배열에서 빼는 이유는, 예시처럼 fetch 함수의 아규먼트로 query들을 전달할 수 있는 화살표 함수로 작성하고자 하는 것인데요. 애초에 이러한 구조로 만드는 것이 적절한지, 또 개선할 지점들이 있을지 궁금합니다.

2번 질문

1번 질문의 예시에서 useQuery에 onSuccess 기능을 추가하고자 한다면 어떤 방식으로 구현할 수 있을까요..? fetch된 데이터는 return 할 수 있지만, useQuery를 불러오는 시점(아규먼트를 작성하는 시점)에서 아규먼트로 전달할 방법을 잘 모르겠습니다.

  const { loading, error, data } = useQuery({
    queryFn: () => getProducts({ ... }),
    deps: [pageSize],
    onSuccess((fetchedData) => {
       ...
    })
  });

[ByungyeonKim]

안녕하세요, 수형님!

와..지난번에 말씀드린 개선 사항들을 모두 깔끔하게 구현해 주셨네요. 👍
주요 변경사항을 자세하게 적어주시고 생각하신 과정들을 꼼꼼하게 설명해주셔서,
제가 피드백을 더 잘 드릴 수 있을 것 같아요!

1. 폴더 구성이 잘 되어 있어요.
   : 정말 깔끔하게 재구성하셨네요. 재사용을 위한 범용 컴포넌트와 페이지 전용 컴포넌트를 잘 분리하신 점이 인상 깊었습니다. 특히 페이지 컴포넌트에서 사용하는 컴포넌트를 가까이 배치하신 부분이 좋았어요. 어떻게 하면 확장성이 좋아질지 고민을 많이 하신 것 같네요. 💯

2. CSS 변경사항
   : 변경하신 이유가 너무 공감이 되네요. 🤣 CSS Modules + SCSS 조합 지금은 어떤가요? 저는 수형님이 Tailwind CSS도 사용해 보시면 좋을 것 같아요. Tailwind CSS는 클래스 이름을 고민할 필요가 없고, JSX 파일 내에서(창을 번갈아가며 확인할 필요없이) 스타일링을 할 수 있다는 장점이 있어요. 성능도 좋고, 적응이 되면 스타일링이 정말 빨라져요. 물론 모든 도구에는 장단점이 있지만 직접 경험해 보시는 것도 좋을 것 같아요. 🙂

3. TanStack Query

• lint 경고 무시
  : react-hooks/exhaustive-deps 린트 규칙은 올바른 방향을 제시하고, 어떤 종속성을 포함해야하는지 알려주죠. 따라서 이 경고를 무시할 경우, 대부분은 설계를 잘못했을 경우가 큽니다. 그렇다고 asyncFunction을 의존성에 추가하면 매개변수를 넘겨야 하는 fetch 함수에선 무한 루프가 발생하구요. 만약 의존성에 추가를 하고 싶다면 호출부에서도 메모이징을 해야해요. 복잡성이 더 커지는 거죠.
• 설계
  : 저는 사실 메모이징 훅을 별로 안 좋아해요. 신경써야하는 부분도 많아지고, 메모가 깨지는 경우도 생겨서 잘 사용하지 않습니다. 버벅이거나, 사용이 필요한 경우를 자주 경험해보지 않아서 그런 것도 있구요. 😊 현재 코드에서 어떻게하면 useCallback을 사용하지 않고 구현해 볼수 있을까에 대한 고민을 많이 했어요. 관련 글도 찾아봤는데, 가장 좋은 방법은 Ref 패턴을 사용하는 거라고 생각해요.

useRef로 asyncFunction의 최신 참조를 유지하고, refetch를 ref에 한 번만 할당하면, deps만으로 재호출 트리거가 가능해져요. 호출부에서 화살표 함수를 사용해도 무한 루프없이 동작하구요! 아래에서 suggestion 코드를 확인해 주세요.

4. onSuccess 기능 추가하기
   : 이번에도 ref를 이용해서 response 데이터가 있을 때만 값을 최신화 시켜주면 구현은 가능할 것 같아요. 하지만 여기서 더 기능을 확장하기 보다는, useQuery를 호출하는 곳에서 이펙트 처리를 하거나, data를 이용해서 값을 계산해도 좋을 것 같아요. 😊

좋습니다! 수형님 덕분에 저도 많은 인사이트를 얻었네요. 고생 많으셨어요~!! 👏

[ByungyeonKim]

Suggested change

	const refetch = useCallback(async () => {
	setState((prev) => ({ ...prev, error: null, loading: true }));
	try {
	const response = await asyncFunction();
	setState((prev) => ({ ...prev, loading: false, data: response }));
	} catch (error) {
	setState({ data: null, loading: false, error });
	}
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [...deps]);
	useEffect(() => {
	if (immediate) refetch();
	}, [immediate, refetch]);
	const asyncFnRef = useRef(asyncFunction);
	useEffect(() => {
	asyncFnRef.current = asyncFunction;
	}, [asyncFunction]);
	const refetchRef = useRef(null);
	if (!refetchRef.current) {
	refetchRef.current = async () => {
	setState((prev) => ({ ...prev, error: null, loading: true }));
	try {
	const response = await asyncFnRef.current();
	setState((prev) => ({ ...prev, loading: false, data: response }));
	} catch (error) {
	setState({ data: null, loading: false, error });
	}
	};
	}
	useEffect(() => {
	if (immediate) refetchRef.current();
	}, [immediate, ...deps]);

이렇게 Ref 패턴을 사용하면 useCallback 훅을 사용하지 않아도 정상 동작해요. React Query에서 실제로 사용하는 방식이기도 합니다. 코드를 천천히 읽고, 분석해 보세요. 🤓

[Ospac]

제안 감사드립니다☺️!
알려주신 내용을 검색하여 몇개의 글을 읽어보았습니다. 좀 어려운 내용들인 것 같네요🥲. 그래서 추가적으로 수정을 하고 있는데, 두가지의 공부 확인겸 질문이 있습니다ㅎㅎㅎ

1.
useRef에 대한 React 문서를 읽어보니, ref.current를 할당할때 렌더링중 한번만 할당하기 위해서 useEffect를 사용하거나, null check 패턴을 사용하는 것 같습니다. 그렇다면 제안 해주신 코드도 일관성을 위해서 다음과 같이 null check로 통일해도 무방할까요? 동작은 동일한 것 같습니다.

  const asyncFnRef = useRef(null);
  if (asyncFnRef.current === null) {
    asyncFnRef.current = asyncFunction;
  }

  const refetchRef = useRef(null);
  if (refetchRef.current === null) {
    refetchRef.current = async () => {
      setState((prev) => ({ ...prev, error: null, loading: true }));
      try {
        const response = await asyncFnRef.current();
        setState((prev) => ({ ...prev, loading: false, data: response }));
      } catch (error) {
        setState({ data: null, loading: false, error });
      }
    };
  }

2.
알고보니 Tanstack Query v5에서는 onSuccess, onError등을 오용 케이스 때문에 deprecated 했다고 하는데요. 해당 글에서 Data Fetching 로직과 Side Effect 로직을 분리하는 코드를 권장하는 내용을 보았습니다. 같은 이유로 현재 코드의 deps 부분도 분리하는게 좋을까요?

사실은 이 코드에서 오용될 여지는 잘 모르겠습니다. 다만, deps에서 react-hooks/exhaustive-deps 경고를 보니, useFetch의 호출부와 useEffect 사이 코드의 거리가 멀고, 여러 상태들을 deps에 넣다 보면, 무한 렌더링에 빠지기 쉬울 수 있을 것 같기도 합니다.

 useEffect(() => {
    if (immediate) refetchRef.current();
  }, [immediate, ...deps]); 

// 현재 useFetch는 deps 받아서 refetch를 실행하고 있지만 해당 매개변수를 제거하고, 
// 호출부에서 다른 custom hook 혹은 useEffect로 Side Effect를 처리하기

[ByungyeonKim]

어려운 내용이지만, 같이 대화해 가면서 알아보죠! 😊

수형님이 제안하신 코드라면, asyncFnRef는 만들지 않아도 되지 않을까요? 그럼 결국 아래의 코드와 같은 동작이 되고, 오래된 클로저(stale closure)가 생길 수 있어요.

 const refetchRef = useRef(null);
  if (refetchRef.current === null) {
    refetchRef.current = async () => {
      setState((prev) => ({ ...prev, error: null, loading: true }));
      try {
        const response = await asyncFunction();
        setState((prev) => ({ ...prev, loading: false, data: response }));
      } catch (error) {
        setState({ data: null, loading: false, error });
      }
    };
  }

그럼 결국, deps가 바뀌어도 최신 응답을 받을 수 없게 돼요. 수형님이 제안하신 코드로 페이지네이션을 테스트 해보면 알 수 있어요. 🙂 (useEffect를 사용한 건, asyncFunction의 응답을 최신값으로 업데이트 하기 위해 effect 안에 넣은 거예요.)

맞아요! V5 버전부터 useQuery에 관련된 콜백들이 제거됐죠. useRef를 통해 refetch의 재생성을 막았기 때문에, deps가 많아져도 무한 렌더링은 되지 않을 거라고 생각해요. 다만, 이제 수형님이 onSuccess와 같은 기능을 추가하기 보다는 글에서 나온 것처럼 effect나 파생 상태로 관리하는 것을 제안드렸던 거예요. 🤓

[Ospac]

생각지도 못하게 stale closure와 closure에 대해서도 공부하게 되었네요ㅋㅋㅋ 감사합니다! (이래서 자바스크립트를 공부해야하는군요 😷)