Code review khj0426 #4
Conversation
|
안녕하세요 신청해주셔서 감사합니다! |
There was a problem hiding this comment.
안녕하세요 효중님! 코드리뷰 신청해주셔서 감사합니다~!
저는 직접 블로그를 구현할 엄두가 안나서 템플릿같은걸 이용해서 만들었었는데, 이렇게 처음부터 끝까지 직접 구현하신게 정말 대단하시네요..! 고생 많으셨습니다!
일단 폴더 구조부터 말씀을 드리면, 전체적으로 깔끔하고 좋습니다!
다만 최근에 저도 느끼게 된 부분이, 꼭 상단에 위치한 폴더에 파일들을 정리할 필요는 없는 것 같아요.
예를 들어 Component 폴더에 서비스에서 사용하는 모든 컴포넌트들을 모아놓으셨는데, 잘못된 부분은 전혀 아닙니다! 저도 예전에 이렇게 했었고, 이 방법에서 불편함을 느낀적 역시 없었습니다.
다만 회사에서 일을 하고, 흔히 말해 규모가 큰 프로젝트를 개발하고 유지보수하면서 느끼게 된 점이, 이렇게 상단에 모든 컴포넌트들을 모아두면, 나중에 파일을 찾는 것이 정말 힘들어지곤 합니다.
물론 검색을 통해서 파일을 찾는 경우가 더 많긴 하지만, 그래도 이름이 기억이 안나거나, 시각적으로 확인하고 싶을 때 파일을 찾는게 쉽지 않아요.
그래서 저도 예전에는 이렇게 종류가 맞는 파일들끼리 모아놓는 폴더 구조를 선호했었는데, 최근에는 관련있는 파일들끼리 가깝게 위치시키는 것을 더 선호합니다.
관련이 있다는 의미는 곧 변경이 발생할 때 같이 변경될 가능성이 높다는 의미이기도 하고, 그러면 가깝게 위치해있으면 변경할 때 더 편하겠죠!
비슷한 이치로 Type들 역시 따로 하나의 폴더에 굳이 정리해놓을 필요는 없을 것 같아요. 아마 저라면 애초에 타입을 따로 파일로 빼지 않고, 컴포넌트 상단에서 선언을 하지 않았을까 싶네요!
https://kentcdodds.com/blog/colocation 를 읽어보시면 도움이 되실 것 같아요!
이 부분들을 제외하고는 코드리뷰를 통해서 제가 평소에 개발할 때 신경쓰는 부분들을 짚어드린 것 같습니다!
이제 남겨주신 고민에 대해서 제 생각을 조금 말씀을 드리자면,
- Next 13에서는 fetch를 사용해 여러 옵션을 줄 수 있고, 해당 옵션에 따라 데이터 불러오는 방식이 달라지는 것으로 알고 있습니다. 이 fetch함수를 실제 쓸 때 어떤 식으로 사용하시는 지 궁금합니다.!
Next의 Page Router를 이용해보셨다면, fetch의 추가 설정들이 SSR, SSG, ISR과 사실상 동일한거라고 생각하시면 편합니다! 다만 SSR, SSG, ISR같은 경우 페이지 단위로 동작했었다면, fetch를 이용하면 컴포넌트 단위로 최적화가 가능합니다.
만약 fetch를 이 프로젝트에서 잘 활용한다면, 블로그 포스트의 로딩속도가 훨씬 더 빨라질 수 있습니다. 빌드타임에 미리 만들어놓을 수 있으니까요!
자세한건 https://nextjs.org/docs/pages/building-your-application/rendering/static-site-generation 또는 Velog 참고하면 도움이 되실거에요!
- 에러 처리 관련해서 고민이 있는데, 개인 계정으로 Sentry를 쓰고 있어요! 다만 Sentry를 처음 쓰고, 기능도 되게 많아서 어떻게 하면 잘 쓸 수 있을지 고민입니다.
그리고 Next를 쓰실 때 에러 처리를 어떤 식으로 하시는지 궁금해요! 알려주시면 제 프로젝트에도 반영하고 싶습니다.. !
Sentry를 사용하면 유저들이 겪은 에러들을 알아낼 수 있다는 점에서 정말 좋은 도구라고 생각합니다! 더불어 특정 에러가 발생하기 전의 상황을 파악할 수 있기에 어디서 언제 에러가 발생했는지, 코드에 어떤 허점이 있는지를 파악하기가 더 쉬워지죠.
저는 개인적으로 Sentry를 두가지 방식으로 사용합니다.
- ErrorBoundary에 잡히는 에러를 로그 남기고 싶을 때
- 가면 안되는 위치에서 코드가 실행될 때 (ex. 분기 처리를 빼먹었을 경우)
질문 주신 Next에서의 에러 처리와도 관련이 있는데, Next App Router에서는 error.tsx 파일을 통해 Error Boundary를 간편하게 설정할 수 있어요! 잘 활용하면 트리의 최하단에서 발생한 에러가 잡히지 않아 서비스 전체를 죽이는 불상사를 방지할 수 있습니다.
마지막으로 가장 중요한 요 프로젝트를 더 고도화 한다면 어떤식으로 풀 수 있을지..? 이력서에 넣어도 될지..?? 고민입니다..에 대해 제 개인적인 의견을 드리자면..
저라면 포트폴리오에 프로젝트로 넣지는 않을 것 같습니다.
그냥 개발 블로그가 있다는걸 알리기 위해 링크는 첨부할 수는 있어도, 이 블로그 자체를 프로젝트로 삼지는 않을 것 같아요.
이유는 여러가지가 있지만, 가장 중요한 두가지만 말씀을 드리자면,
-
이력서/포트폴리오에 넣는 프로젝트의 수는 다다익선이 아니라고 개인적으로 생각합니다.
10개의 프로젝트를 했다고 10개를 다 넣는게 아닌, 그중에서 가장 자신이 있고, 가장 배운게 많은, 그리고 가장 눈에 띄는 프로젝트 서너개만 넣는게 좋다고 생각합니다.
너무 많이 넣으면 면접관 입장에서도 다 보기 싫어질 수도 있고, 이 사람은 하나를 깊게 하지 않는구나라는 잘못된 편견이 생길까봐 우려가 되기도 하고, 그 서너개의 프로젝트 자리에는 여러 팀원들과 함께해서 완성도 높게 만든 프로젝트가 들어가야 좋을 것 같아요. 그래야 협업에 대한 장점도 동시에 어필할 수 있으니까요! -
개발 블로그는 너무나도 흔한 프로젝트입니다.
요즘 개발자들은 너도나도 본인의 블로그를 만들어서 글을 적고 운영을 하기 때문에 블로그를 프로젝트로 삼아서 줄 수 있는 차별점이 많지가 않아요.
글을 정말 잘적고 깊이 있는 내용을 다루거나, 블로그의 디자인 또는 기능이 상상 이상인 경우 말고는 블로그가 특출나보이기는 쉽지 않다고 생각해요.
효중님께서 작성하신 블로그 글들을 읽어보았는데, 꽤 흥미롭고 깊이있는 글들이 많았어서 되게 재밌게 읽었습니다! 다만 아무리 회사에 취업하면 디자이너분들이 계셔서 디자인을 잘할 필요는 없다고 하지만, 프론트엔드 개발자는 보여지는 부분들 만드는 역할이기 때문에 디자인적인 감각이 어느정도는 중요하다고 생각합니다. 그리고 효중님의 블로그 디자인에서는 그런 디자인 감각이 어필이 되지 않는다고 생각했어요. 저 역시 그런 디자인 감각이 부족한 사람이기에 개인적인 블로그를 만드는 것이 어려울거라 판단했고, 그렇기에 저는 Velog를 사용하거나, 온라인에서 찾을 수 있는 블로그 디자인 템플릿을 사용했었습니다. 장점이 아니라면 굳이 드러내지 않는게 좋지 않을까 싶어요! 장점만 보여줘도 힘든게 요즘 취업 시장이니까요..!
좋은 프로젝트, 그리고 좋은 블로그 만드느라 고생 정말 많으셨습니다. 새로운 글 읽으러 자주 방문할게요!!
취업 준비 응원합니다!!
| const posts = slugs | ||
| .map((slug) => getPostBySlug(slug, fields)) | ||
| // sort posts by date in descending order | ||
| .sort((post1, post2) => (post1.date > post2.date ? -1 : 1)); |
There was a problem hiding this comment.
내림차순으로 정렬을 하고 싶은 경우 sort((a, b) => b - a) 를 하면 됩니다!
이 경우 sort((post1, post2) => post2.date - post1.date)가 될 것 같네요.
| const posts = slugs | ||
| .map((slug) => getPostBySlug(slug, fields)) | ||
| .sort((post1, post2) => (post1.date > post2.date ? -1 : 1)) | ||
| .slice(0, 5); | ||
|
|
||
| return posts; | ||
| } | ||
|
|
||
| export function getAllPostRequest(fields: string[] = []) { | ||
| const slugs = getPostSlugs(); | ||
| const posts = slugs | ||
| .map((slug) => getPostBySlug(slug, fields)) | ||
| // sort posts by date in descending order | ||
| .sort((post1, post2) => (post1.date > post2.date ? -1 : 1)); |
There was a problem hiding this comment.
Post를 받아오고 내림차순하는 로직이 여러번 반복되고 있는 것 같아요!
코드의 반복을 개선할 수 있는 방법이 있지 않을까요??
There was a problem hiding this comment.
해당 코드의 반복을 막기 위해
Post를 받아오고 내림차순으로 반환하는 역할을 하는 함수를 만들고 이 함수를 재사용하면 될 것 같아요!
혹시 더 좋은 방법이 있을까요?!?!
| allPosts.map((post) => { | ||
| const getCategory = allCategory.get(post.category); | ||
| if (getCategory) { | ||
| allCategory.set(post.category, getCategory + 1); | ||
| return; | ||
| } | ||
|
|
||
| allCategory.set(post.category, 1); | ||
| }); |
There was a problem hiding this comment.
map과 forEach의 차이를 한번 생각해보시면, 이 상황에서는 map이 적절한지, forEach가 적절한지 알 수 있을 것 같아요!
There was a problem hiding this comment.
지금 상황에서는 forEach가 더 적당한 것 같네요..!! 감사합니당
| return Array.from(allCategory).map(([category, categoryCount]) => { | ||
| return { | ||
| category: category, | ||
| categoryCount: categoryCount + '', | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
객체를 바로 반환하려고 의도하신 것 같아요! 이렇게 할 수도 있지 않을까요??
Array.from(allCategory).map(([category, categoryCount]) => (
{
category: category,
categoryCount: categoryCount + '',
}
));
| const posts = slugs | ||
| .map((slug) => getPostBySlug(slug, fields)) | ||
| // sort posts by date in descending order | ||
| .sort((post1, post2) => (post1.date > post2.date ? -1 : 1)) |
| export default function TOC({ toc }: { toc: string[] }) { | ||
| const { id } = useToc(); | ||
|
|
||
| const TOC = toc.map((eachToc) => { | ||
| const makeTOC = replaceStrWithBlank([eachToc, ['#', '##', '###', '####']]); | ||
|
|
||
| const handleClickTOC = (e: React.MouseEvent<HTMLElement>) => { | ||
| const activeElement = document.querySelector('.active'); | ||
| if (activeElement) { | ||
| activeElement.classList.remove('active'); | ||
| } | ||
| e.currentTarget.classList.add('active'); | ||
| }; | ||
| return ( | ||
| <> | ||
| <li key={uuid4()}> | ||
| <div> | ||
| <StyledTOCLink | ||
| className={`${makeTOC} ${id === makeTOC && 'active'}`} | ||
| href={`#${makeTOC}`} | ||
| id={makeTOC} | ||
| onClick={handleClickTOC} | ||
| > | ||
| {makeTOC} | ||
| </StyledTOCLink> | ||
| </div> | ||
| </li> | ||
| </> | ||
| ); | ||
| }); | ||
|
|
||
| return <StyledTOCList>{TOC}</StyledTOCList>; | ||
| } |
There was a problem hiding this comment.
이 컴포넌트의 경우 리렌더 한번한번이 꽤 비싸지 않을까 싶은데, 최적화를 하면 좋을 것 같아요!
React.memo()또는 React.useMemo()를 잘 활용해보시면 어떨까요?
물론 그러려면 prop으로 전달되는 toc역시 useMemo처리가 잘 되어있어야 합니다!
| }: { | ||
| recordMap: PromiseType<ReturnTypeofNotionRecord>; | ||
| }) { | ||
| const [modeState] = useRecoilState(themeState); |
There was a problem hiding this comment.
theme의 상태값만 필요하고, setter함수는 필요가 없을 것 같은데, 그런 상황에서 사용하는 Recoil 훅이 따로 있을 것 같습니다!
There was a problem hiding this comment.
useRecoilValue로 수정했습니다!! 감사해요!!
| {data && | ||
| data.guestbook && | ||
| Array.from(Object.values(data.guestbook)).map((value) => ( | ||
| <div key={uuidv4()}>{value.comment}</div> |
| Array.from(Object.values(data.guestbook)).map((value) => ( | ||
| <div key={uuidv4()}>{value.comment}</div> | ||
| ))} |
There was a problem hiding this comment.
매번 배열을 만들고 map을 하는 것보단, 따로 변수로 빼서 useMemo를 활용해도 좋을 것 같고, 아예 따로 컴포넌트화 시켜도 좋을 것 같아요!
저라면 GuestBookList가 children을 받는게 아닌, prop으로 배열을 받고, 내부에서 리스트를 렌더링을 시킬 것 같아요! 그리고나서 React.memo를 잘 활용하면 성능 측면에서 좋지 않을까 싶습니다!
There was a problem hiding this comment.
말씀하신 것처럼 GuestBookList가 배열을 받도록 하는게 좋을 것 같아서 수정하였습니다!!!
//GuestBookList
type guestBookListProps = {
id: string;
comment: string;
commentTime: string;
};
const GuestBookList = ({
guestbookList,
}: {
guestbookList: guestBookListProps[];
}) => {
return (
<StyledGuestBookList>
{guestbookList.map((value) => {
return (
<Entry key={value.id}>
<EntryContent>{value.comment}</EntryContent>
</Entry>
);
})}
</StyledGuestBookList>
);
};
export default GuestBookList;props로 내리는 곳에선 다음과 같이 수정해보았어요!!!
'use client';
import { useMemo } from 'react';
import GuestBookInput from '@/Component/GuestBook/GuestBookInput';
import GuestBookList from '@/Component/GuestBook/GuestBookList';
import useGetGuestBook from '@/hooks/queries/useGuestBookQuery';
export default function GuestBook() {
const { data, refetch } = useGetGuestBook();
const guestBookList = useMemo(() => {
if (data && data.guestbook) {
return Array.from(Object.entries(data.guestbook)).map((value) => {
return {
comment: value[1].comment,
commentTime: value[1].commentTime,
id: value[0],
};
});
}
return [];
}, [data]);
return (
<main>
<GuestBookList guestbookList={guestBookList} />
<GuestBookInput refetch={refetch} />
</main>
);
}There was a problem hiding this comment.
useMemo를 활용하여 데이터를 캐싱한 것은 너무나도 좋습니다!! 👍 👍
그럼 여기서 한단계 더 나아가서, guestBookList를 prop으로 전달받는 컴포넌트의 리렌더링을 최적화 하려면 어떻게 할 수 있을까요??
There was a problem hiding this comment.
답변이 늦어져서 죄송함니다ㅜ
guestBookList를 props로 받는 컴포넌트의 리렌더링을 최적화하려면 memo를 쓸 것 같아요.!!
궁금한점이 하나 있는데, 컴포넌트에 memo를 쓰면 props가 바뀌지 않은 한 컴포넌트가 기억된다구 알고 있는데,
지금 같은 경우는 query의 응답값으로 받아온 객체 배열의 props를 넘겨주는 방식이다보니,
결국 guestBookList가 새 방명록이 올라올 때마다 렌더링이 다시 되더라구요..!
그러다 보니 memo를 쓰는게 맞을까? 어느 상황에서 memo를 써야하지? 라는 고민이 생겼습니다.!.! 😢
어느 상황에서 memo를 쓰는게 맞을까요./.?? 궁금함니다..
There was a problem hiding this comment.
맞습니다! memo를 활용하면 prop이 변경되지 않은 컴포넌트들은 리렌더링이 되지 않습니다.
질문을 제가 잘 이해하지 못한 것 같아 여쭤봅니다!
guestBookList의 데이터 업데이트 되어 변경이 된 경우를 말씀하시는게 맞을까요??
만약 그게 맞다면 guestBookList의 데이터가 변경이 됨에 따라 리렌더링이 되어야하는게 맞을 것 같습니다! 데이터가 변경이 되었는데 리렌더링이 되지 않는다면 변경된 데이터가 화면에 반영이 되지 않기에 오히려 그게 더 버그에 가깝다고 볼 수 있겠죠.
memo를 통해 성능적인 이점을 누릴 수 있는 상황을 생각해보면,
memo를 사용하지 않는다면 매번 리렌더링이 발생하는 상황을
memo를 이용하여 몇몇 상황들에서 리렌더링을 스킵하여 리렌더링 횟수를 줄이는 데에 이점이 있다고 볼 수 있습니다!
따라서 memo를 사용해야하는 경우는
- prop이 자주 바뀌지 않는 컴포넌트일때
- prop을 아예 받지 않는 컴포넌트일 때
- 컴포넌트의 연산이 무거운 상황일 때
이 세가지를 고려하면 좋을 것 같습니다.
반대로 prop이 너무 자주 변경되는 컴포넌트거나, 컴포넌트가 가벼워 리렌더링이 발생해도 별로 문제가 되지 않을 경우에는 꼭 쓰지는 않아도 된다고 생각합니다!
memo를 통한 최적화 작업은 시행착오를 겪으면서 본인만의 기준이 생기게 되니, 한번쯤 신경을 쓰면서 개발을 해보시면 기준을 잡는데에 도움이 되실 것 같습니다!
There was a problem hiding this comment.
감사합니다!! 언제 memo를 써야하는지 잘 알게 된 것 같아요 !!😄
| export default function replaceStrWithBlank([input, target]: [ | ||
| string, | ||
| string | string[] | ||
| ]) { | ||
| if (target instanceof Array) { | ||
| const result = target.map((eachTarget) => { | ||
| return input.replaceAll(eachTarget, ''); | ||
| }); | ||
| return result[0].trim(); | ||
| } | ||
|
|
||
| return target.replaceAll(input, ''); | ||
| } |
There was a problem hiding this comment.
아마 주어진 input 문자열에서 target 문자열 또는 target 배열안에 있는 문자열을 input 문자열에서 지우는 함수인 것 같은데, 구현에 오류가 있지 않을까 싶습니다.
우선 target이 array인 경우, 각 target에 대해서 map을 하고, input.replaceAll(eachTarget, '')를 합니다. 그리고 최종적으로 result[0].trim()을 반환하는데, 0번째 인덱스에 있는 문자열은 모든 target을 지운 문자열이 아닌, 첫번째 target만 지운 문자열입니다.
그리고 두번째, target이 string인 경우 target.replaceAll(input, '')를 하는데, 이 경우 target 문자열에서 input을 찾아서 바꾸는 것이기에 역할이 뒤바뀐 상태입니다.
따라서 만약 제가 이 함수를 구현하려 했다면,
export default function removeTargetFromInput(input: string, target: string | string[]) {
if (target instanceof Array) {
return target.reduce((currentInput, eachTarget) => {
return currentInput.replaceAll(eachTarget, '');
}, input);
}
return input.replaceAll(target, '');
}
이런식으로 구현하지 않았을까 싶네요!
함수명 역시 조금 더 직관적으로 target을 input에서 없애는걸 보여주려고 할 것 같고, 값들을 굳이 배열로 받지 않을 것 같습니다!
Pull Request 템플릿
배포
로컬 환경
리뷰 요청 ✍️
- 신청하게 된 계기, 간단한 사연
깃허브에서 보게 되어서 신청하게 되었어요!
정말 취준생 그 자체이고, 취업을 하고 싶은 사람으로서, 현업분의 시선에서 제 코드를 볼 수 있는 기회인 것 같아서 신청합니다.
메일로 환경변수 보냈습니다.!!!
- 중점적으로 리뷰받고 싶은 부분
제 사이트는 여러 기록을 남기고, 볼 수 있는 개발 블로그입니다!
아무래도 혼자서만 한 프로젝트이다 보니, 이걸 다른 사람이 보았을 때, 읽기 쉬울까? 컴포넌트를 다시 재활용할 수 있나?
라는 고민을 많이 하게 됩니다.
재사용가능하게 코드를 짠다면, 나중에 제가 다시 몇개월 뒤에 코드를 보아도, 누군가가 이 코드를 보기에도 쉬울 것 같아서,
해당 부분을 리뷰 받고 싶었어요!
추가적으로 이건 진짜 이상한거 같은데 <- 싶은거 모두 다 리뷰받고 싶습니다
- 나누고 싶은 고민
Next 13에서는 fetch를 사용해 여러 옵션을 줄 수 있고, 해당 옵션에 따라 데이터 불러오는 방식이 달라지는 것으로 알고 있습니다. 이 fetch함수를 실제 쓸 때 어떤 식으로 사용하시는 지 궁금합니다.!
에러 처리 관련해서 고민이 있는데, 개인 계정으로 Sentry를 쓰고 있어요! 다만 Sentry를 처음 쓰고, 기능도 되게 많아서 어떻게 하면 잘 쓸 수 있을지 고민입니다.
그리고 Next를 쓰실 때 에러 처리를 어떤 식으로 하시는지 궁금해요! 알려주시면 제 프로젝트에도 반영하고 싶습니다.. ! 🥲