-
Notifications
You must be signed in to change notification settings - Fork 37
[남기연] sprint9 #308
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
[남기연] sprint9 #308
The head ref may contain hidden characters: "Next-\uB0A8\uAE30\uC5F0-sprint9"
Conversation
1005hoon
left a comment
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.
기연님, nextjs / ts로 변환하시면서 오류가 발생한다 하셨는데 그래도 되는 부분들에 대해서 잘 작성해주셨어요!
일단 전반적으로 기능 구현하는 측면에서 큰 어려움은 없어보여요!
그리고 타입 시스템도 적절하게 활용하고 계시구 있구요.
이제 개선시킬 수 있는 부분이 크게 세가지로 보여요!
- 콤포넌트의 크기 줄이기
- 너무 복잡해진 타입들 펼쳐주기
- 타입 이름 엔티티 기반으로 명확하게 구분하기
콤포넌트 크기 줄이기의 경우, 지금 한 콤포넌트가 너무 많은 작업을 처리하는 경우가 많이 보이고 있습니다. 따라서 큰 콤포넌트가 다 처리하도록 하기보단, 작은 단위의 역할이 명확한 콤포넌트 여럿으로 분리시켜서 그 콤포넌트들을 묶어주는 형태로 풀어보면 아마 훨씬 가독성이 높은 코드로 탈바꿈 할 수 있으리라 생각해요.
그리고 타입 시스템의 경우, type alias를 어떻게든 활용하고자 다양한 타입들을 만들어 사용하는 현상이 보이고 있는데요. 사실 타입스크립트의 타입 시스템은 정말 필요한 타입이 아니라면 굳이 만들지 않는걸 추천하기는 합니다. 따라서 뭔가 타입이 계속 복잡하게 만들어지고, 타입을 위한 타입이 만들어지거나, 타입 이름이 되게 직관적이지 않게 느껴진다면 한번 내가 콤포넌트의 prop을 너무 복잡하게 구조화 하지는 않았나 한번 살펴볼 수 있으면 좋아요.
마지막으로 타입 이름 엔티티 기반으로 구분하기의 경우, 우리가 ~~Data, ~~ListData와 같이 데이터임을 표현하는 식으로 타입 이름들이 설정되어있어요. 근데 타입스크립트의 경우 기본적으로 타입 시스템을 적용함에 있어서 객체지향적인 코드 형태를 따른다고 말씀드렸었죠? 이 관점에서 우리가 객체로 평가할 요소들을 살펴보면, 등록된 제품들, 제품에 대한 댓글들 등이 있을거에요. 이런 요소들을 타입으로 평가해보면 Product, Comment 등으로 평가할 수 있을거고,
이에 대해서 데이터를 불러왔을때의 결과물인 경우, GetProductResponse, GetCommentResponse 와 같이 새로운 타입들을 지정해 활용해볼 수 있을거에요.
자세한 피드백들은 코드리뷰에 남겨두었으니 한번 참고 부탁드리구
지속적으로 프로젝트 해 나가시면서, 고민되는 부분 또는 걱정되거나 해결하기 어려운 부분이 있다면 디스코드로 언제든지 질문주셔요!
고생 많으셨습니다
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.
👍🏻
next/src/api/api.ts
Outdated
| pageSize = 3, | ||
| }: getBestPostsDataProps) { | ||
| const response = await fetch( | ||
| `https://panda-market-api.vercel.app/articles?page=1&pageSize=${pageSize}&orderBy=${orderBy}` |
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.
page size와 order조건 외에도 다양한 패러미터들을 전달할 수 있을텐데요,
이런 요소들을 다 묶어서 타입 설정을 해보면 어떨까요? .
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.
search parameter들의 경우, 영문자와 숫자가 아닌 경우 서버를 옮겨다니면서 �query들이 깨질 수 있어요
이를 안전하게 처리하기 위해서 javascript 내장 모듈인 URLSearchParams를 사용하는걸 추천드려요
next/src/api/api.ts
Outdated
| const response = await fetch( | ||
| `https://panda-market-api.vercel.app/articles?page=1&pageSize=${pageSize}&orderBy=${orderBy}` | ||
| ); | ||
| if (!response) { |
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.
음 사실 response 데이터가 아예 없는 경우는 없답니다!
따라서 네트워크 오류에 대한 핸들링을 하고자 한다면 response.ok 를 판단해주어야 해요
https://developer.mozilla.org/ko/docs/Web/API/Response/ok
next/src/api/api.ts
Outdated
| `https://panda-market-api.vercel.app/articles?page=1&pageSize=${pageSize}&orderBy=${orderBy}` | ||
| ); | ||
| if (!response) { | ||
| throw new 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.
또한 서버에서 오류가 발생했다면 오류 메시지를 전달하게 될텐데 지금의 경우
서버에서 반환해주는 오류 메시지를 로깅하는 곳이 없답니다.
따라서 실제 배포 또는 테스트 환경에서 구동할때 오류가 발생한다 해도 디버깅 하기가 상당히 어려워질거에요.
적절한 오류 핸들링을 한번 해보면 좋겠습니다!
next/src/api/api.ts
Outdated
| throw new Error("베스트 게시글 페칭에 실패"); | ||
| } | ||
| const data = await response.json(); | ||
| return data.list; |
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.
data list만 전달하게 되면 사실 몇번째 페이지 데이터인지 판단하기가 어렵잖아요
이에 대한 데이터들을 다 반환해주면 어떨까 싶긴 하지만
최초 n개의 데이터만 보여주면 되는 로직이다 보니 굳이 그렇게 구현할 필요가 없어보이기도 하네요!
next/src/types.ts
Outdated
| export interface PostData { | ||
| id: number; | ||
| title: string; | ||
| content: string; | ||
| image: string | null; | ||
| likeCount: number; | ||
| createdAt: string; | ||
| updatedAt: string; | ||
| writer: { | ||
| id: number; | ||
| nickname: string; | ||
| }; | ||
| } | ||
|
|
||
| export interface PostListData { | ||
| list: PostData[]; | ||
| totalCount: 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.
다음과 같이 구분지어보면 어떨까요?
interface Post { .. }
interface GetPostResponse {
list: Post[]
totalCount: 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.
👍🏻
| const pageSizeCheck = () => { | ||
| const pageWidth = window.innerWidth; | ||
| if (pageWidth < 480) { | ||
| setPageSize(1); | ||
| } else if (pageWidth >= 480 && pageWidth < 768) { | ||
| setPageSize(2); | ||
| } else { | ||
| setPageSize(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.
이런 ui차원의 로직은 한번 분리시켜보면 어떨까요?
export function useMediaQuery() { ... }
const { isMobile, isTablet, isDesktop } = useMediaQuery()
useEffect(() => {
if(isMobile) setPageSize(...)
if(isTablet) ...
if(isDesktop) ...
}, [])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.
useMediaQuery에서 resize 이벤트에 대한 구독을 해주고, 넓이에 따라 현재 어떤 기기인지 판단해주는 로직을 구현해두는거죠
| const fetchBestPosts = async () => { | ||
| const posts = await fetchPosts({ orderBy: "like", pageSize }); | ||
| setBestPosts(posts); | ||
| }; | ||
| fetchBestPosts(); |
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.
꼭 이렇게 래퍼 함수를 둘 필요는 없구,
useEffect(() => {
fetchPosts({ ... })
.then(setBestPosts)
.catch( ... )
}, [])로 처리해도 괜찮습니다
|
|
||
| export const getServerSideProps = async () => { | ||
| const initialPosts = await fetchPosts({ orderBy: "like", pageSize: 3 }); | ||
|
|
||
| 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.
이거는 한번 페이지 사이즈나 orderby와 같은 설정값들을 어떻게 전달할 수 있을지 한번 고민해보면 어떨까요
1005hoon
left a comment
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.
되게 리뷰사항들 잘 반영해주고 계시네요!
이번 이어서 올라오게 될 pr도 기대됩니다!
next/src/types.ts
Outdated
| export interface OrderByValue { | ||
| orderByValue: "recent" | "like"; | ||
| } |
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 type PostListOrderOption = 'recent' | 'like '| throw new Error(`Failed to fetch, ${response.statusText}`); | ||
| } | ||
| const data: PostListData = await response.json(); | ||
| const data: Posts = await response.json(); |
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.
우리 멘토링떄 이야기했던것처럼 list 반환타입의 경우 제네릭을 활용해서 거기에 list필드가 반환하는 값이 Post[] 가 될 수 있도록 한번 해볼까요?
|
|
||
| export default function AllPosts({ initialAllPosts }: AllPostProps) { | ||
| const [postList, setPostList] = useState<PostData[]>(initialAllPosts); | ||
| const [postList, setPostList] = useState<Post[]>(initialAllPosts); |
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.
[postList, ...] 보다 마찬가지로 해당 엔티티 그 자체를 사용해 posts라고 변수선언을 해보면 어떨까요?
| const [orderBy, setOrderBy] = useState<"recent" | "like">("recent"); | ||
| const [filterPostList, setFilterPostList] = | ||
| useState<PostData[]>(initialAllPosts); | ||
| const [filterPostList, setFilterPostList] = useState<Post[]>(initialAllPosts); |
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.
filter값이 바뀔때마다 리렌더링 되다보니 그냥
const [filter, setFilter] = useState('')
const filteredPosts = posts.filter(post => post.title.includes(filter))
return { fileredPosts.map( ... ) }하는식으로 하는걸 추천드려요!
| isDesktop: boolean; | ||
| }; | ||
|
|
||
| export function useMediaQuery(): MediaQuery { |
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.
오 커스텀 훅 사용하신거 상당히 좋습니다.
조금만 더 정리해본다면, 지금 상황의 경우, pageSize 가 객체로 데이터를 주다보니 결국 활용하는 입장에서 이 결과를 또 다시 어디ㅓㄴ가 풀어 사용해야할텐데요.
그냥 이런 경우 직관적으로 풀어서 필드 하나씩 때려주는것도 괜찮은 방법이에요.
개인적으로 사용하는 코드 참고 차원에서 첨부드립니다
"use client";
import { useEffect, useState } from "react";
function getDevice(): "mobile" | "tablet" | "desktop" | null {
if (typeof window === "undefined") return null;
return window.matchMedia("(min-width: 1024px)").matches
? "desktop"
: window.matchMedia("(min-width: 640px)").matches
? "tablet"
: "mobile";
}
function getDeminsions() {
if (typeof window === "undefined") return null;
return { width: window.innerWidth, height: window.innerHeight };
}
export function useMediaQuery() {
const [device, setDevice] = useState<"mobile" | "tablet" | "desktop" | null>(
getDevice(),
);
const [dimensions, setDimensions] = useState<{
width: number;
height: number;
} | null>(getDeminsions());
useEffect(() => {
const checkDevice = () => {
setDevice(getDevice());
setDimensions(getDeminsions());
};
checkDevice();
window.addEventListener("resize", checkDevice);
return () => {
window.removeEventListener("resize", checkDevice);
};
}, []);
return {
device,
width: dimensions?.width,
height: dimensions?.height,
isMobile: device === "mobile",
isTablet: device === "tablet",
isDesktop: device === "desktop",
};
}
요구사항
기본
심화
주요 변경사항
스크린샷
홈화면에서 /boards로 이동합니다.





최신순
좋아요순
검색기능
화면크기에 따라 게시글 갯수 변경
멘토에게