-
Notifications
You must be signed in to change notification settings - Fork 37
[김찬기] Sprint12 #330
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
[김찬기] Sprint12 #330
The head ref may contain hidden characters: "Next-\uAE40\uCC2C\uAE30-sprint12"
Conversation
- 전부 클라이언트 사이드에서 처리하도록 교체 (기존은 서버액션)
let cachedClientAccessToken: string | null = null; | ||
|
||
const getAccessTokenAtServer = cache(async () => { | ||
const session = await auth(); | ||
return session ? session.accessToken : null; | ||
}); | ||
|
||
const getAccessTokenAtClient = async () => { | ||
if (!cachedClientAccessToken) { | ||
const session = await getSession(); | ||
return session ? session.accessToken : null; | ||
} | ||
return cachedClientAccessToken; | ||
}; | ||
|
||
axiosInstance.interceptors.request.use( | ||
async (config) => { | ||
const session = | ||
typeof window === "undefined" ? await auth() : await getSession(); | ||
const accessToken = session?.accessToken; | ||
const accessToken = | ||
typeof window === "undefined" | ||
? await getAccessTokenAtServer() | ||
: await getAccessTokenAtClient(); | ||
|
||
if (accessToken) { | ||
config.headers.Authorization = `Bearer ${accessToken}`; | ||
} else { | ||
delete config.headers.Authorization; |
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.
이전 코드리뷰에서 말해주신 내용중에, 매번 session을 요청하는것에 대해서 이렇게 개선해보았습니다.
- 클라이언트 사이드에서 토큰을 헤더에 넣을때는 캐시된것이 있으면 재사용하고,
- 서버사이드에서 토큰을 헤더에 넣을때에는 매번 새로운 요청으로 토큰을 받아오도록 했습니다.
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.
고생하셨어요ㅎㅎ!
컴포넌트를 깔끔하게 잘 만드셔서 그런지 재사용하기도 편하고 확장성도 열려있어서 좋네요
일관성도 있어서 덕분에 코드리뷰도 편하게 했네여!
감사합니다~!!
import ProductForm from "@/components/market/ProductForm"; | ||
import { useProductAdd } from "@/service/product.queries"; | ||
|
||
export default function AddItemPage() { | ||
const { mutateAsync: handleProductAdd } = useProductAdd(); | ||
|
||
return ( | ||
<PageWrapper> | ||
<ProductAddForm /> | ||
<ProductForm mode="add" onFormSubmit={handleProductAdd} />; | ||
</PageWrapper> | ||
); |
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.
좋은 구조인것같아요...! 추가인지 수정인지에 따라 로직 수정도 쉽고 좋네여!
조금 더 개선한다면 ProductForm에 isLoading 같은 props를 받아서 로딩처리도 같이 할 수 있으면 좋겠네여ㅎㅎ!
아 그리고 외부에 submit 버튼이 있는경우에도 사용할 수 있으려면 formId를 받아서 처리하는것도 방법이에요
if (isPending) { | ||
return <Loading>게시물 정보를 가져오는 중입니다.</Loading>; | ||
} | ||
|
||
if (!detail) { | ||
notFound(); | ||
} |
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.
useGetArticle 에서 enabled 조건이 없어서 isPending 이 처음부터 true 인 경우엔 의도하신대로 동작할것같아요!
근데 만약에 enabled에 조건이 들어가서 바로 fetching이 되지 않는 경우엔 isPending이 false 가 되고, defail은 undefined라서 notFound 페이지로 빠질것같아요.
isFetched 상태도 같이 활용하는것도 방법일것같구요
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.
생각을 못했던 부분인것 같습니다. 감사합니다! 다시 한번 작성해보겠습니다.
detail: Product; | ||
} | ||
|
||
export default function ProductDetail({ detail }: ProductDetail) { | ||
const { | ||
id, | ||
images, | ||
name, | ||
price, | ||
description, | ||
tags, | ||
ownerId, | ||
ownerNickname, | ||
updatedAt, | ||
favoriteCount, | ||
isFavorite, | ||
} = detail; | ||
const { data: session } = useSession(); | ||
export default function ProductDetail() { | ||
const router = useRouter(); | ||
const { handleLike, handleProductDelete } = useProductActions(id); | ||
const isOwner = ownerId === Number(session?.user?.id); | ||
const { data: session } = useSession(); | ||
const { id } = useParams<{ id: string }>(); | ||
const productId = Number(id); |
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.
이렇게 해도 틀린건 아니고 사실 많이 이렇게 하긴 해요ㅎㅎ;;
components/.../ProductDetail.tsx 이니 가급적 props를 통해서 id 값을 넘겨받는게 어떨까 싶긴 해요
외부 의존성을 최대한 줄이고, props로 받아와서 로직적인 부분에 대한 제어를 부모쪽에서 하는게 재사용하기 좋은 구조가 될꺼에요~!
id값을 number로 부모에서 받으면 좋을것같아요!
if (confirm("정말 삭제할까요?")) { | ||
try { | ||
await handleArticleDelete(); | ||
alert("상품을 삭제했습니다."); | ||
await deleteArticle(); | ||
alert("게시글을 삭제했습니다."); | ||
router.replace("/boards"); | ||
} catch (err) { | ||
console.log(err); | ||
console.error(err); |
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.
관심사를 분리하는쪽으로 로직을 나누면 좋을것같아요!
deleteArticle이 성공했을때와 실패했을때를 각각 나누었다면 차라리 여기선 useArticleDelete 에서 onSuccess 와 onError 를 callback으로 받아서 처리할 수 있도록 하면 어떨까 싶어요ㅎㅎ!
원래 useMutaion에 있는 onError와 onSuccess 처럼요
} | ||
await handleLike(!isLiked); | ||
router.refresh(); | ||
toggleLike(!isLiked); |
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.
이미 알고 계시긴 할텐데, 습관적으로 이전 상태값 기준으로 상태를 변경하면 좋긴 해요ㅎㅎ;;
(너무 사소하긴 하지만ㅠ)
toggleLike(prev => !prev);
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.
toggleLike가 useMutation의 mutate함수를 이름을 다르게 지정해서 쓴거라서
usestate의 기존 상태값을 통해서 상태변경을 하는 방식을 못썼습니다.
제가 다르게 이해한건지, 한번 더 체크해볼겠습니다!
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.
useMutation의 onMutate 콜백에서 이전 상태값을 기준으로 변경하는 방식으로 다시 작업해보겠습니다!
} | ||
|
||
return ( | ||
<> |
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.
아마 lint에 잡혔을것같긴 한데, 자식이 1개일떈 <>...</> 는 지워도 될것같아요!
} catch (error) { | ||
const message = isAxiosError(error) | ||
? error.response?.data.message | ||
: "알 수 없는 에러가 발생했어요."; | ||
|
||
throw new Error(message); |
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.
이부분은 반복되는 내용들이 조금 있는것같아서 유틸쪽으로 합쳐도 괜찮을것같네여ㅎㅎ!
// https://github.com/nextauthjs/next-auth/issues/9465 | ||
// redirect false로 응답을 받아볼때, 로그인실패도 ok가 true 전달되고 있음 | ||
// 임시로 message와 코드로 실패처리 | ||
if (response?.error === "CredentialsSignin") { | ||
throw new Error(response.code); |
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.
오... 정말 그러네요;; ㄷㄷ
주석으로 이슈가 되는 링크도 그렇고 대처도 그렇도 너무 좋네요ㅎㅎ!
useComments(Number(id), name, data); | ||
const { isLoading, error, data, hasNextPage, fetchNextPage } = useGetComments( | ||
name, | ||
{ id: Number(id), limit: 3 } |
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.
여기도 만약 id를 props로 받아오면 조금더 의존성을 줄일 수 있을것같아요~
아 그리고 좋은점이 만약 id값이 이상하게 들어오는 경우를 대비하여 NaN 검사도 해야하는데, 그러한 로직을 부모 컴포넌트로 올리고 CommentList.tsx 는 로직에만 집중할 수 있다는 장점도 생기겠네요ㅎㅎ!
let cachedClientAccessToken: string | null = null; | ||
|
||
const getAccessTokenAtServer = cache(async () => { | ||
const session = await auth(); | ||
return session ? session.accessToken : null; | ||
}); | ||
|
||
const getAccessTokenAtClient = async () => { | ||
if (!cachedClientAccessToken) { | ||
const session = await getSession(); | ||
return session ? session.accessToken : null; | ||
} | ||
return cachedClientAccessToken; | ||
}; | ||
|
||
axiosInstance.interceptors.request.use( | ||
async (config) => { | ||
const session = | ||
typeof window === "undefined" ? await auth() : await getSession(); | ||
const accessToken = session?.accessToken; | ||
const accessToken = | ||
typeof window === "undefined" | ||
? await getAccessTokenAtServer() | ||
: await getAccessTokenAtClient(); | ||
|
||
if (accessToken) { | ||
config.headers.Authorization = `Bearer ${accessToken}`; | ||
} else { | ||
delete config.headers.Authorization; |
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.
너무 좋은데요ㅎㅎ! 잘 생각해두셨네여ㅋㅋ
요구사항
기본
중고마켓
상품 상세
상품 등록
심화
주요 변경사항
스크린샷
멘토에게