[김우성] Good-Night-Hackathon-Frontend 제출 #7
Conversation
There was a problem hiding this comment.
안녕하세요 우성님.
조금 늦었지만 이제야 코드리뷰를 남겨드립니다.
코드의 전반적인 구조와 변수 명칭, 그리고 함수의 추상화가 잘 되어있어 재사용과 유지보수를 다소 고려하셨다고 생각했습니다. 복잡한 로직을 간결하게 추상화 하신부분도 좋았던거 같아요.
개인적으로 충분히 코드 짜는 역량을 기르실 수 있을 것 같아 욕심이 나서 사소한 부분까지 리뷰를 남겨드렸습니다...
하나 강조드리고 싶은 부분이 있다면 전역 상태 관리 부분이 과연 이 프로젝트에서 꼭 필요했을지 한번 더 고민해보셨으면 좋겠습니다.
사용자 및 어드민 권한에 대한 부분은 그럴 수 있지만, 서버 데이터의 경우 불필요하게 전역적인 상태관리가 일어나고 있다고 생각했거든요.
api호출을 자주 해도 상관없으니, 필요한 페이지에서 최신데이터를 호출하는 방식으로 코드를 개선하는 방식으로 수정해보시길 권장드립니다.
또한 시간적 여유가 되신다면 react-query도 사용해보셔서 서버데이터에 대한 관심사도 분리해보면 좋을 것 같아요.
코드 리뷰가 조금이나마 도움이 되셨길 바랍니다. 추가적으로 질문이나 더 알고 싶은 사항이 있다면 언제든지 말씀해 주세요.
| import viteLogo from '/vite.svg'; | ||
| import './App.css'; | ||
|
|
||
| function App() { |
There was a problem hiding this comment.
App.tsx가 잘못된 방법으로 사용되고있습니다.
아마 기본템플릿이 그대로 사용된 것 같은데요,
App.js 는 애플리케이션의 메인 컴포넌트 파일로 main.tsx에 구현된 내부 컴포넌트 요소를 App.tsx로 옮겨주세요.
또한 작성하신 main.tsx에서는 단순히 App컴포넌트를 랜더링하는 �하는 역할을, index.html은 React컴포넌트를 마운트하도록 처리하는 역할만 수행하도록 파일을 분리시키는게 좋을 것 같습니다.
해당 구조는 React에서 기본적인 구조로 사용하는 패턴이기때문에 염두해두시길 권장드립니다.
향후 프로젝트 규모가 커진다면 가독성과 유지보수성 측면에서 차이가 드러날뿐아니라 일괄된 방식으로 확장할 수 있는 이점이 있기때문입니다.
참고용 공식문서 참고해주세요.
|
|
||
| const useAuthorityStore = create<AuthorityStore>((set, get) => ({ | ||
| authority: false, | ||
| setAuthority: () => { |
There was a problem hiding this comment.
상태 업데이트 코드에서 set을 사용해 새로운 상태를 정의하도록 구현되어있는데,
Zustand에서는 이전 상태값을 받아와 상태를 업데이트하는게 가능합니다.
import { create } from 'zustand'
const useBearStore = create((set) => ({
bears: 0,
increasePopulation: () => set((state) => ({ bears: state.bears + 1 })),
removeAllBears: () => set({ bears: 0 }),
}))
공식문서에서 사용된 코드 업데이트의 예시인데요,
직접 상태값을 가져오지 않고 이전값을 활용하는 방식은 불변성 유지 및 데이터흐름을 명확히 드러낼 수 있어 권장되는 패턴입니다.
특히 여러 상태업데이트가 동시에 발생할때 저렇게 코드가 구현되어있다면 값이 의도한되로 반영되지 않을 가능성이 있습니다.
"읽어온 시점"의 state를 기준으로 값을 변경하기때문에 변경이 발생하기전에 값을 읽어오는 상황이 생길수도 있기때문입니다.
const useAuthorityStore = create<AuthorityStore>((set) => ({
authority: false,
setAuthority: () => {
set((state) => ({ authority: !state.authority }));
},
}));
따라서 다음과 같이 코드를 수정하는것을 권장드립니다.
| import bgImg from '../../assets/tree.png'; | ||
|
|
||
| const BackLayout: React.FC = ({ children }) => { | ||
| return <div className="w-[100vw] h-[1034px]">{children}</div>; |
There was a problem hiding this comment.
1034px와 같이 고정된 높이를 props기반의 동적처리로 수정해도 괜찮을 것 같네요..!
| @@ -0,0 +1,14 @@ | |||
| import react from '@vitejs/plugin-react-swc'; | |||
There was a problem hiding this comment.
잘못 삽입된 import 구문인것 같습니다.
실제 코드상에서도 사용되고있지 않고 React의 경우 아래 방식으로 import할 수 있습니다.
import React from 'react';
| @@ -0,0 +1,14 @@ | |||
| import react from '@vitejs/plugin-react-swc'; | |||
|
|
|||
| const NavBar: React.FC = ({ children }) => { | |||
There was a problem hiding this comment.
children의 대해서도 타입명시가 가능합니다.
맥락상 다양한 React 요소(다른 컴포넌트, 문자열 등)를 지원할 수 있도록 React.ReactNode 타입으로 처리하시면 될 것 같습니다.
| import axios from 'axios'; | ||
|
|
||
| const WishConfirmPage: React.FC = () => { | ||
| const { wishList } = useWishStore(); // wishList를 가져옵니다. |
There was a problem hiding this comment.
wishList의 데이터가 잘못된 맥락으로 사용되고 있는 것 같습니다.
코드를 봤을떄는.. 다른 페이지에서 호출한 wishList데이터를 화면상에 표현하고자 사용하신 것 같다고 생각했는데,
이 경우 데이터의 동기화 및 불일치 문제가 일어날 수 있습니다.
처음부터 해당 페이지에 들어온다던가 오랜 시간이 지나 해당 페이지에서 작업을 처리하는 등의 사례가 있을 것 같아요.
적어도 이 경우 해당 페이지에 접속했을때 소원 목록을 가져오는 api를 호출하고, 상태를 동기화하는 작업을 수행하는게 필요할 것 같습니다.
전역적으로 관리했을때 큰 이점도 없는 변수인것같아요..!
특히나 이런 서버데이터의 경우 React-query라는 라이브러리를 사용해보시는것도 권장드립니다.
| <div className="m-auto w-[50%] h-[50%] flex flex-col justify-center"> | ||
| <p className="text-[32px] font-bold mb-[2vh]">소원열매 승인</p> | ||
| {wishList | ||
| .filter((wish) => wish.isConfirmed === null) // 필터링 조건을 적용합니다. |
There was a problem hiding this comment.
서버에서 필터링된 데이터를 받아온다고 알고있습니다.
해당 필터링 조건은 빼도 괜찮을 것 같아요..!! (만약 서버에서 �정상적으로 필터링된 데이터를 받아올 수 없다면 서버측에 수정을 요청해야지 프론트 내에서 처리하는건 좋지 않은 것 같습니다.)
| import { GotWish } from '../interface/Wish'; | ||
| import axios from 'axios'; | ||
|
|
||
| const WishGetPage: React.FC = () => { |
There was a problem hiding this comment.
wishId값또한 path로부터 값을 바로 가져오도록 처리하는게 좋을 것 같아요!!
굳이 전역에서 해당 id값을 관리할 필요는 없을 것 같습니다.
<Route path="/get/:id" element={<WishGetPage />} />
const { id } = useParams<{ id: string }>();
특히 이런 path로부터 값을 가져온다면 새로고침을 하거나 특정 링크를 접속했을때 의도한 페이지를 바로 보여줄 수 있다는 장점이 있습니다.
| const { wishList, findByWish, setWishList, wishId, setWishId, resetAll } = | ||
| useWishStore(); | ||
| const { authority, setAuthority } = useAuthorityStore(); | ||
| const [state, setState] = useState<boolean>(false); |
There was a problem hiding this comment.
state보다 명확한 의도가 드러나는 변수명을 사용해주세요.
이 경우 "wishPage"로 이동한다는 의도를 가지고 해당 변수를 사용하신 것 같은데 useEffect에서 값을 체크하고 이동하는 로직은 useEffect의 안티패턴입니다.
handleClickWish 내부에서 useNavigate를 사용해 직접 이동코드를 구현하는게 좋아요~~!
const navigate = useNavigate();
const handleClickWish = () => {
setWishId(id);
handleRedirect('url~~~');
};
useState의 경우 비동기적으로 상태가 업데이트되고, useEffect에서는 화면의 렌더링을 마친 후 사이드이펙트로 값을 감지하고 이동로직을 수행하기때문에 성능측면에서도 다소 차이가 날 수 있습니다.
| navigate(url); | ||
| }; | ||
| useEffect(() => { | ||
| resetAll(); |
There was a problem hiding this comment.
resetAll()함수를 사용하지 않는게 더 좋을 것 같습니다.
상태 업데이트가 빈번하면 해당 상태를 구독중인 컴포넌트에서는 재랜더링이 발생해 성능측면에서도 좋지 않고, 이 경우 의도상 "이전 데이터를 보여주지 않기 위해 사용" 했다고 생각했기 때문입니다.
오히려 이런 경우,
api데이터에 대한 로딩 변수를 통해 응답이 반환되기전까지 로딩 UI를 별도로 분리해 표현한다면 샤용자 경험 개선에도 크게 도움이 되지 않을까 싶네요
No description provided.