-
Notifications
You must be signed in to change notification settings - Fork 166
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
[개인 미션 - 성능 오답노트] 가브리엘(윤주현) 미션 제출합니다. #70
Conversation
- GitHub Pages 배포하기를 위한 설정 - 의존성 패키지 관리
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 메세지로 올려주신 내용들 확인해보니 다 정상적으로 성능이 나오네요!
고생 많으셨어요~
접근성을 신경쓰신 부분도 인상 깊었습니다 ㅎㅎ
대부분 저랑 비슷하게 하셔서 코드 리뷰하는데 어려움 없었습니다.
저와 다르게 한 부분에 대해서는 궁금증이 있어서 코멘트에 질문 남겨놨어요~
딱히 고칠 부분은 보이지 않습니다!
질문에 답변 해주시면 바로 approve 하겠습니다! 고생하셨어요~
<meta charset="UTF-8"/> | ||
<meta http-equiv="X-UA-Compatible" content="IE=edge"/> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"/> | ||
<meta name="description" content="memegle for wooteco"/> |
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.
description meta 태그 좋네요~
const Home = lazy(() => import('./pages/Home/Home')); | ||
const Search = lazy(() => import('./pages/Search/Search')); |
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.
저도 Home과 Search를 분리해주었는데, Home에 대해서는 Lazy를 걸어줄 필요가 있을까란 다른 크루들의 의견도 있더라구요~ 가브리엘은 어떻게 생각하시나요?
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.
저도 이 부분에 대해서 고민을 해보았는데, Home에서만 접근이 가능한 홈페이지의 경우에는 Home을 lazy-loading 처리하지 않는 것이 나은 것 같습니다. Home을 초기 렌더링하는 과정에서 Loading이 보이느라 홈페이지가 오히려 느려보이는 효과가 있는 것 같아요.
하지만 다른 위치에서 접속이 가능한 성격의 홈페이지라면 둘다 분리하는 것이 나은 것 같아요. 예를 들면 사용자가 링크를 외부로 공유해서 Search 페이지로 초기에 접근하는 경우에는 Home이 필요 없을테니깐요...!
@@ -21,16 +21,25 @@ function convertResponseToModel(gifList: IGif[]): GifImageModel[] { | |||
}); | |||
} | |||
|
|||
const cache: { trending: GifImageModel[] } = {trending: []}; |
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.
전역 변수로 cache를 설정하셨군요! 좋습니다~ 이 부분도 어떤 크루들은 setTime으로 캐싱 시간도 설정해주었더라구요~ 참고하시면 좋을 것 같습니다~
<div className={styles.projectTitle}> | ||
<h1 className={styles.title}>Memegle</h1> | ||
<h3 className={styles.subtitle}>gif search engine for you</h3> | ||
<h2 className={styles.subtitle}>gif search engine for you</h2> |
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.
h2로 변경한 이유가 궁금합니다!
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.
heading-order에 관한 규칙으로는 1씩 증가하는 것을 권장하고 있습니다. heading은 페이지의 의미 구조를 전달하므로 보조 기술(스크린 리더, SEO 등)을 사용할 때 1씩 증가하는 것이 낫다고 합니다.
이렇게 하면 lighthouse 점수도 올릴 수 있습니당
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.
오호 하나 배워갑니다👍🏻
<FeatureItem title="See trending gif" imageSrc={trendingGif} /> | ||
<FeatureItem title="Find gif for free" imageSrc={findGif} /> | ||
<FeatureItem title="Free for everyone" imageSrc={freeGif} /> | ||
<FeatureItem title="See trending gif" videoSrc={trendingVideo}/> |
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명까지 바꿔주신거 좋아요~
|
||
return ( | ||
<li className={styles.artistContainer}> | ||
<img className={styles.profileImage} src={profileImageUrl} /> | ||
<img className={styles.profileImage} src={profileImageUrl} alt="artist info image"/> |
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.
접근성을 위해 alt까지 세심하시네요 ㅎㅎ
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 { AiOutlineInfo, AiOutlineClose } from 'react-icons/ai'; | ||
import {useState} from 'react'; | ||
import {AiOutlineInfo} from '@react-icons/all-files/ai/AiOutlineInfo'; | ||
import {AiOutlineClose} from '@react-icons/all-files/ai/AiOutlineClose'; |
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.
저도 all-files를 써주긴 했는데, production으로 빌드 시 알아서 tree-shaking이 된다고 하더라구요? 이 부분에 대해서 아시는게 있으실까요?
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.
그러게요. 제가 이 부분은 측정한 자료를 캡쳐해두지 않아서 정확하지 않을 수 있지만
react-icons/react-icons#154 (comment)
해당 이슈의 코멘트에서는 all-files 를 사용해서 번들링 사이즈를 줄였다고 하거나
https://www.npmjs.com/package/react-icons 에서는
If your project grows in size, this option is available. This method has the trade-off that it takes a long time to install the package.
라고 되어있다는 자료를 보았습니다.
@@ -2,3 +2,5 @@ declare module '*.png'; | |||
declare module '*.jpg'; | |||
declare module '*.gif'; | |||
declare module '*.svg'; | |||
declare module '*.webp'; | |||
declare module '*.webm'; |
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.
webm 확장자를 사용해주셨네요~ mp4 선택지도 있을텐데 webm을 사용하신 이유가 궁금합니다!
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.
홈페이지에서 사용된 움짤 자체가 고화질의 영상이 필요하지 않은 점을 고려했을 때 비교적으로 손실률이 높고 압축률이 높은 webm을 선택하는 것이 낫다고 생각했습니다. 비교적 저화질이므로 용량이 줄고 로드 시간을 줄여줄 것으로 기대했습니다!
new MiniCssExtractPlugin(), | ||
new CssMinimizerPlugin() |
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.
저는 눈에 띄게 번들링 사이즈가 줄어 드는거 같질 않아서 css 번들링을 따로 분리해주지 않았어서 잘 모르는데 두개의 플러그인 차이를 혹시 알 수 있을까요??
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.
- MiniCssExtractPlugin: js 번들에서 css파일을 분리하는데 사용됩니다.
css를 별도의 파일로 추출하는 것은 여러가지 이점이 있는데요, css파일이 따로 있기에 웹 브라우저에서 (용량이 줄어든)JS파일과 함께 병렬로 로딩이 가능하게 됩니다. 또, css파일이 별도로 존재하므로 웹브라우저단에서 캐싱할 수도 있습니다. 이건 해보지는 않았지만 css 파일도 여러 개로 쪼개는 기능이 있다고 합니다.
- CssMinimizerPlugin: css 파일을 압축할 때 사용됩니다.
다만 저희 미션에서 사용된 css는 크기가 크지 않아 체감하기 힘들 것이라고 생각해요.
optimization: { | ||
minimize: true, | ||
minimizer: ['...', new CssMinimizerPlugin()], | ||
splitChunks: { |
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.
SplitChunkPlugin이 webpack5에는 기본적으로 내장되어있는걸까요?!
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.
공식 문서에서도 포함되어있고, webpack repository에 들어가봐도 관련 코드가 전부 들어있는 것으로 보아 내장되어있는 것 같습니당
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.
상세한 답변 감사합니다 가브리엘~
덕분에 알아가는게 많았습니다!
첫 리뷰에서 말씀드렸던 것처럼 답변해주셔서 이만 approve하고 merge할게요!
고생많으셨어요👍🏻
🔥 결과
개선 전 (GitHub Pages)
개선 후 (CloudFront)
✅ 개선 작업 목록
1 요청 크기 줄이기
2 필요한 것만 요청하기
3 같은 건 매번 새로 요청하지 않기
4 최소한의 변경만 일으키기
🧐 공유
좋은 리뷰 미리 감사합니다.