-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat(article): 아티클 상세 페이지를 코멘트 영역을 제외하고 실제 데이터와 연동합니다. #125
Conversation
- 아티클 상세화면 쪽 slug 를 articleSlug 로 리네이밍했습니다.
@@ -1,4 +1,5 @@ | |||
import axios, { AxiosRequestConfig } from 'axios'; | |||
import type { AxiosRequestConfig } from 'axios'; |
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 type { AxiosRequestConfig } from 'axios'; | |
import axios, { type AxiosRequestConfig } from 'axios'; |
이렇게도 가능하답니다!
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.
아하 이렇게도 되는군요! VS 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.
이거 저장할 때마다 위 처럼 자동 포맷팅이 되어서; 현재 해둔대로 계속 가야할 듯 하네요ㅠ eslintrc
에 추가한 룰 때문에 그런가 싶기도 하구요 👀
@@ -0,0 +1,6 @@ | |||
export interface ArticleAuthor { |
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.
양 API 에서 동일하게 쓰이는 응답이라 별도 분리 해 주었습니다. 다만 Article.types.ts
라는 파일 이름이 적절한지는 고민이었어요. (api
혹은 service
라는 뉘앙스가 들어가야 하지 않을까 싶었고 그냥 ArticleService.types.ts
에 있는게 맞나 싶기도 했구요)
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.
컴포넌트 props로도 사용될 수 있는 타입이라면, API에 의존적인 타입이 아니라 전역 타입으로 정의해서 가져다 사용하는 도메인 타입으로 분리해도 좋답니다!
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.
컴포넌트 props로도 사용될 수 있는 타입이라면, API에 의존적인 타입이 아니라 전역 타입으로 정의해서 가져다 사용하는 도메인 타입으로 분리해도 좋답니다!
도메인 타입 쪽으로도 계속 고민이었는데.. 그 방향으로 다른 작업할 때에 한 번 시도해봐야겠습니다!
@@ -0,0 +1,72 @@ | |||
const ArticleComments = () => { |
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 에서 API 연동까지 포함해 개선할 예정입니다.
import { formatDate } from '../../utils/dateUtils'; | ||
|
||
interface ArticleHeaderProps { | ||
article: ArticleDetailData; |
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.
다른 곳들은 실제 필요한 데이터만 props 에 선언하도록 해 두었는데, 여기는 전달해야 할 데이터가 꽤 많아서 이런 형식으로 해 보았습니다. 사실 사용이 이게 훨씬 편해서 이걸로 다 통일할까.. 싶기는 했어요. 어차피 단방향 통신인지라 컴포넌트에서 전체 데이터를 받는다고 해서 문제될 요소는 크게 없다는 생각이긴 했습니다.
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.
이게 리액트 컴포넌트가 props를 핸들링하는 부분에서 좋지 않은 퍼포먼스를 부르게 되는 안티 패턴입니다.
기본적으로 props는 불변으로 관리되어야 합니다. 그런데 객체는 참조 형태이므로, 객체 내부 값이 변경되면 부모-자식 컴포넌트에 문제가 생길 가능성이 있어요.
또한 객체의 변화를 감지해야 하므로 렌더링 비용이 증가될 수 있어요!
가급적이면 객체 프로퍼티를 분해하여 원시 형태 데이터를 내려주도록 신경 쓰는 것이 좋아요.
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.
이게 리액트 컴포넌트가 props를 핸들링하는 부분에서 좋지 않은 퍼포먼스를 부르게 되는 안티 패턴입니다.
기본적으로 props는 불변으로 관리되어야 합니다. 그런데 객체는 참조 형태이므로, 객체 내부 값이 변경되면 부모-자식 컴포넌트에 문제가 생길 가능성이 있어요.
또한 객체의 변화를 감지해야 하므로 렌더링 비용이 증가될 수 있어요! 가급적이면 객체 프로퍼티를 분해하여 원시 형태 데이터를 내려주도록 신경 쓰는 것이 좋아요.
오 렌더링 비용과 관련이 있는 부분이군요! 그렇다면 항상 분해해서 전달하는 식으로 앞으로 개발해야겠습니다 ㅎㅎ
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에서 바로 수정했습니다!
|
||
if (!articleDetail) { | ||
return null; | ||
} |
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.
최초 구현시에는 Loading UI 를 표시하도록 해 두었었는데, prefetch 로직을 추가하게 되면서 이걸로 바꾸었습니다. 처음에는 프래그먼트(<></>
) 를 넣었었는데 null 이랑 특별히 차이가 없는 듯해서 이걸로 했습니다.
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.
좋습니다! null이 <></>
보다 성능이 좋아요👍
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.
좋습니다! null이
<></>
보다 성능이 좋아요👍
오 성능 차이가 실제로 있었군요..! 코멘트 감사합니다 ㅎㅎ
createdAt={articleDetail.article.createdAt} | ||
favorited={articleDetail.article.favorited} | ||
favoritesCount={articleDetail.article.favoritesCount} | ||
/> |
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가 바뀔 때마다 매번 props 를 수정해야 하는 불편함이 생깁니다. 이건 Best Practice 가 뭔지 좀 궁금하네요..!
cc @InSeong-So
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.
보통 API 데이터가 어떻게 구성되어 있는지에 따라 컴포넌트를 분리하거나 합칠 지 결정하면 됩니다.
만약 내부 컴포넌트 UI 변경이 부모에 의존적이지 않고 변칙적이라면(articleDetail 데이터 내부 값이 아니라 특수한 값을 인자로 받아야 한다면) 이렇게 작업해주는 것이 좋고, 아니라면 부모에서 호출하는 데이터와 똑같이 구조 분해 할당을 통해 데이터를 내려주는 방향을 고민해보면 좋을 것 같습니다.
또는 부모 데이터 > 자식 데이터의 통합을 통해 핸들링하거나, 아니면 API 데이터를 핸들링할 때 컴포넌트에 내려줄 데이터만 분리해서 관리하는 것도 방법이에요.
구조적으로 best practice는 없습니다! 방법론과 패턴의 차이거든요💪
<ArticlePreviewLink href={articleLink}> | ||
<ArticlePreviewLink | ||
href={articleLink} | ||
onClick={e => { |
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.
이렇게 href 와 onClick 을 같이 두는게 적절한 방법인지 모르겠네요. 밑에서 e. preventDefault()
처리를 하는게 좋은 방법일지..
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.
통일 시키는 것을 추천드립니다! 기본적으로 a태그는 지양해주는 것이 좋습니다.
프레임워크에서 제공하는 태그 컴포넌트를 활용하거나, 이벤트 핸들러로 일관되게 만들어주세요👍
href는 브라우저 자체를 갱신하는 반면, 프레임워크가 제공하는 기술은 변경되는 부분만 갈아끼우는 형태로 최적화 되어 있기 때문입니다!
그리고 인라인 함수는 컴포넌트가 렌더링 될 때마다 새로 생성되기 때문에, 가급적 부모 컴포넌트에서 함수를 선언하여 변수를 주입하는 형태로 작성해주세요!
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.
@InSeong-So 여기 수정을 조금 해보았는데요, 부모에서는 이미 handleArticleClick 을 주입해 주고 있기는 한데 함수 클로져의 타입이 달라서 컴포넌트 내부에서 한 번 재처리를 해야 하기는 합니다.
아래와 같은 두 방식이 구현이 가능한 것 같은데 혹시 더 추천하시는 방향이 있으실까요~?
일단 .bind()
가 컴포넌트 리렌더링시에도 함수가 새로 생성되지 않는 방향이라고 해서 더 좋으려나 싶기는 했습니다 ㅎ
const handleArticlePreviewLinkClick = () => {
onArticleClick(articleSlug, articleLink);
};
const handleArticlePreviewLinkClick2 = onArticleClick.bind(
null,
articleSlug,
articleLink,
);
...
<ArticlePreviewLink onClick={handleArticlePreviewLinkClick2}>
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.
우선 2번 방향으로 수정했습니다!
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.
그리고 a 태그 대신에 react-router-dom 의 Link
를 써보려고 했는데, to
에 path 만 지정하는 식으로 현재 처리를 해둔게 아니고 원래의 handleClick 이 prefetch 를 하는 로직이 있어서 바로 변경은 어렵겠다 싶어서 다른 방향을 찾아보았습니다.
다음으로는 button 태그로의 변경을 해보려고 했는데 스타일 처리를 너무 많이 해야 하는 것 같고 정렬도 바뀌길래, 그냥 div 태그에 cursor 주는 식으로 변경했습니다!
() => ArticleService.fetchArticleDetail(articleSlug), | ||
); | ||
navigate(articleLink); | ||
window.scrollTo(0, 0); // 전처리 후 페이지 이동시 스크롤 위치가 최상단으로 안 가있는 문제 수정 |
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.
이게 없으면 순간적으로 페이지 이동 시 스크롤이 내려가 있는 현상이 발생합니다. 최초에는 AppRoutes 쪽에서 ScrollToTop
같은 로직을 만들어 처리하는 쪽으로 구현을 했었는데, 현재로선 특수한 처리라고 봐야 하는데 모든 라우팅에 적용하는게 향후 히스토리 관리 차원에서도 적절하지 않을 듯해 여기에 남기게 되었습니다.
export function AppRoutes(): ReactElement {
return (
<Layout>
<Global styles={globalStyles} />
<Router>
<Routes>
<Route path="/" element={<HomePage />} />
<Route path="/login" element={<LoginPage />} />
<Route path="/register" element={<RegisterPage />} />
<Route path="/profile/:username" element={<ProfilePage />} />
<Route path="/editor/:articleSlug?" element={<EditorPage />} />
<Route path="/article/:articleSlug" element={<ArticlePage />} />
<Route path="/settings" element={<SettingsPage />} />
</Routes>
<ScrollToTop />
</Router>
</Layout>
);
}
function ScrollToTop() {
const location = useLocation();
useEffect(() => {
window.scrollTo(0, 0);
}, [location]);
return null;
}
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.
location은 객체라서, location.pathname 등 원시값을 비교할 수 있게 변경해주면 좋을 것 같아요!
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.
location은 객체라서, location.pathname 등 원시값을 비교할 수 있게 변경해주면 좋을 것 같아요!
오 그렇게 비교하는게 더 정확하군요..! 위 코드는 실제로 들어가지는 않게 된 코드인데 향후 location 사용하게 될 때 참고하겠습니다 💪
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.
폴더 이름이나 파일 위치가 조금 고민되긴 했는데 이것 역시 Best Practice 를 알고 싶네요 ㅎㅎ
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.
일반적으로 Route라고 작성하지만 요 네이밍은 원하시는대로 가져가면 됩니다!
@@ -1,4 +1,5 @@ | |||
import axios, { AxiosRequestConfig } from 'axios'; | |||
import type { AxiosRequestConfig } from 'axios'; |
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 type { AxiosRequestConfig } from 'axios'; | |
import axios, { type AxiosRequestConfig } from 'axios'; |
이렇게도 가능하답니다!
@@ -0,0 +1,6 @@ | |||
export interface ArticleAuthor { |
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.
컴포넌트 props로도 사용될 수 있는 타입이라면, API에 의존적인 타입이 아니라 전역 타입으로 정의해서 가져다 사용하는 도메인 타입으로 분리해도 좋답니다!
} catch (error) { | ||
if (isAxiosError(error) && error.response) { | ||
throw error.response.data as ArticleDetailErrorResponse; | ||
} | ||
throw 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.
try-catch 시 에러를 다음 단계로 아무런 절차 없이 던지는 것은 fetchArticleDetail
을 호출하는 레벨에서 다시 한 번 에러 핸들링을 시도해야 하기 때문에 로컬에서의 에러 핸들링이 의미가 사라지게 됩니다 😅
물론 여기서 로깅이 들어간다면 유의미한 try-catch 구문이에요!
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 type { ArticleAuthor } from '../../apis/article/Article.types'; | ||
import { formatDate } from '../../utils/dateUtils'; |
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.
tsconfig.json
, vite.config.ts
에서 정의할 수 있는 path Alias를 찾아보고 적용하신다면 상대/절대 경로의 스트레스에서 벗어날 수 있습니다!
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.
tsconfig.json
,vite.config.ts
에서 정의할 수 있는 path Alias를 찾아보고 적용하신다면 상대/절대 경로의 스트레스에서 벗어날 수 있습니다!
오 이런 방법이 있군요..! 저는 보통은 자동완성으로 import 를 넣고 있었어서 스트레스를 받지 않는 영역이긴 했습니다 ㅋㅋ;
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 에서 두 군데 alias 설정 해서 아티클 관련 파일들에서 path 설정해 보았습니다!
<textarea | ||
className="form-control" | ||
placeholder="Write a comment..." | ||
></textarea> |
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.
복붙 과정이었겠지만, 이런 태그들은 셀프 클로즈 태그로 활용할 수 있습니다 🥹
<textarea />
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.
복붙 과정이었겠지만, 이런 태그들은 셀프 클로즈 태그로 활용할 수 있습니다 🥹
<textarea />
오 네네 이번 작업에서는 컴포넌트 단 로직을 아예 안 건드렸는데 향후 작업에 참고하겠습니다 ㅎㅎ
|
||
if (!articleDetail) { | ||
return null; | ||
} |
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.
좋습니다! null이 <></>
보다 성능이 좋아요👍
createdAt={articleDetail.article.createdAt} | ||
favorited={articleDetail.article.favorited} | ||
favoritesCount={articleDetail.article.favoritesCount} | ||
/> |
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.
보통 API 데이터가 어떻게 구성되어 있는지에 따라 컴포넌트를 분리하거나 합칠 지 결정하면 됩니다.
만약 내부 컴포넌트 UI 변경이 부모에 의존적이지 않고 변칙적이라면(articleDetail 데이터 내부 값이 아니라 특수한 값을 인자로 받아야 한다면) 이렇게 작업해주는 것이 좋고, 아니라면 부모에서 호출하는 데이터와 똑같이 구조 분해 할당을 통해 데이터를 내려주는 방향을 고민해보면 좋을 것 같습니다.
또는 부모 데이터 > 자식 데이터의 통합을 통해 핸들링하거나, 아니면 API 데이터를 핸들링할 때 컴포넌트에 내려줄 데이터만 분리해서 관리하는 것도 방법이에요.
구조적으로 best practice는 없습니다! 방법론과 패턴의 차이거든요💪
<ArticlePreviewLink href={articleLink}> | ||
<ArticlePreviewLink | ||
href={articleLink} | ||
onClick={e => { |
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.
통일 시키는 것을 추천드립니다! 기본적으로 a태그는 지양해주는 것이 좋습니다.
프레임워크에서 제공하는 태그 컴포넌트를 활용하거나, 이벤트 핸들러로 일관되게 만들어주세요👍
href는 브라우저 자체를 갱신하는 반면, 프레임워크가 제공하는 기술은 변경되는 부분만 갈아끼우는 형태로 최적화 되어 있기 때문입니다!
그리고 인라인 함수는 컴포넌트가 렌더링 될 때마다 새로 생성되기 때문에, 가급적 부모 컴포넌트에서 함수를 선언하여 변수를 주입하는 형태로 작성해주세요!
() => ArticleService.fetchArticleDetail(articleSlug), | ||
); | ||
navigate(articleLink); | ||
window.scrollTo(0, 0); // 전처리 후 페이지 이동시 스크롤 위치가 최상단으로 안 가있는 문제 수정 |
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.
location은 객체라서, location.pathname 등 원시값을 비교할 수 있게 변경해주면 좋을 것 같아요!
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.
일반적으로 Route라고 작성하지만 요 네이밍은 원하시는대로 가져가면 됩니다!
…핸들러를 미리 선언해서 사용하도록 개선
📌 이슈 링크
📖 작업 배경
🛠️ 구현 내용
ArticlePreviewService
네이밍을 잘못 한 이슈를 발견해 수정했습니다.ArticleService
를 작성합니다. 또한 프리뷰 쪽과 author 는 공용으로 쓰고 있어 별도 파일로 분리했습니다.useArticleDetailQuery
도 작성합니다.Routes
를react-router-dom
v6 스타일로 수정합니다. 사실 slug 쪽 이슈만 수정하면 되었었는데, (이렇게 작업을 하면 안되겠지만) 하는 김에 같이 수정했습니다. 그러면서 이름도AppRoutes
로 변경했습니다. (네이밍 중복이 있었습니다)utils/
에dateUtils.ts
를 작성했습니다. 모노레포다 보니 나중에는realworld-library
와 같은 별도 workspace 로 분리해서 사용하도록 개선을 해 보고 싶네요.queryClient.prefetchQuery
와react-router-dom
의useNavigate
두 개를 활용해 먼저 데이터를 읽고 페이지를 이동시키도록 했습니다. 이렇게 하니까 의도한 대로 동작하는 것 같네요!💡 참고사항
🖼️ 스크린샷
article.mov