Skip to content
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

[개인 미션 - 성능 오답노트] 야미(이다인) 미션 제출합니다. #109

Merged
merged 43 commits into from
Sep 11, 2023

Conversation

feb-dain
Copy link

@feb-dain feb-dain commented Sep 7, 2023

안녕하세요 클린! 첫 페어였는데 첫 리뷰어로 만났네요.
정성스러운 리뷰 기대하겠습니다 ^0^

🔥 결과

image
image

  • 개선 후 (CloudFront)

image
image

✅ 개선 작업 목록

자세한 성능 개선 과정 및 설명은 아래 블로그를 참고해 주세요!
https://feb-dain.tistory.com/27

1 요청 크기 줄이기

  • 소스코드 크기 줄이기
    image

  • 이미지 크기 줄이기

image

👇🏻

image

2 필요한 것만 요청하기

  • 페이지별 리소스 분리
    image

  • 아이콘 패키지 Tree Shaking
    image

3 같은 건 매번 새로 요청하지 않기

  • CloudFront 캐시 설정 (설정값, 해당 값을 설정한 이유 포함)
    • 최대 TTL은 1년, 기본 TTL은 하루, 최소 TTL은 1초로 설정했습니다. (기본 캐시 설정)
    • mp4, webp 등 이미지, 비디오 파일은 max-age를 1년으로 설정해 주었습니다. 많이 바뀌지 않을 것이기 때문입니다.
    • response header 정책도 설정해 주었습니다. 변경 사항이 없을 것이라 생각해 1년으로 했습니다.
image
  • GIPHY의 trending API를 Search 페이지에 들어올 때마다 새로 요청하지 않아야 한다.
    image

4 최소한의 변경만 일으키기

  • 검색 결과 > 추가 로드시 추가된 목록만 새로 렌더되어야 한다.
    • React memo 사용했습니다.
  • Layout Shift 없이 애니메이션이 일어나야 한다.
    • top, left... 대신 transform 사용했습니다.
  • Frame Drop이 일어나지 않아야 한다.
    • (Chrome DevTools 기준) Partially Presented Frame 역시 최소로 발생해야 한다.
      image

🧐 공유

  • ❓저는 사용자의 90% 이상이 Home에서 먼저 접근할 거라고 생각해 Home에는 React lazy를 적용하지 않았는데, 클린은 이 부분에 대해서 어떻게 생각하시나요?
  • ❓️최적화를 위해 memo를 사용했는데, 무작정 memo를 쓰는 것은 지양해야 한다고 들었습니다. 언제 memo를 쓰면 좋은지 클린만의 기준이 있나요?
  • ❓️ 클로저를 사용하면서 let을 썼는데, const를 사용해서 객체 안의 프로퍼티를 바꾸는 방식과 고민했습니다. 혹시 클린은 어떤 방식을 선호하시나요? 이유도 궁금합니다!
  • ❓성능 최적화하면서 어떤 게 가장 힘들었나요?
  • ❓클린이 자랑하고 싶은 부분 있나요?

- Common JS 모듈은 트리 쉐이킹 적용 불가
  => 바벨이 모듈을 CommonJS 모듈로 바꾸지 않고 그대로(ES6) 유지하도록 설정
@feb-dain feb-dain self-assigned this Sep 7, 2023
Copy link

@hozzijeong hozzijeong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요?

반갑습니다 야미 감회가 새롭군요!

코드 리뷰를 하면서 야미의 디테일함에 한번 감탄하고 갑니다.👍

몇 가지 궁금한 사항이 있어서 코멘트 남겨보겠습니다!

403 에러 발생

image

Search 페이지로 바로 접근했을 때 search 파일을 받아오지 못하고 있는데, (해당 페이지에서 새로고침 해도 같은 현상이 나타납니다) 아마 cloudFront에서 에러페이지 처리가 안되어 있는것 같아요!! 한번 확인해 보시면 좋을 것 같습니다.

response header 정책

response header 정책으로 cache-control을 1년으로 설정하셨던데, 그렇다면 새롭게 빌드한 index.html 파일은 어떻게 업데이트를 해야 하나요? 해시 값 설정을 통한 캐시 버스팅으로 다른 파일들은 캐시에 저장되지 않는다고 해도 index.html은 이름이 같기 때문에 업데이트 되지 않을것이라고 생각이 되는데 야미의 생각이 궁금합니다!

transform

top, left 대신에 transform을 사용하신 이유가 있나요?


❓저는 사용자의 90% 이상이 Home에서 먼저 접근할 거라고 생각해 Home에는 React lazy를 적용하지 않았는데, 클린은 이 부분에 대해서 어떻게 생각하시나요?

  • 저도 야미와 똑같이 생각해서 Home에서 lazy를 적용하지 않았습니다. 하지만 에슐리가 Home에도 lazy를 적용하지 않으면, 다른 페이지에 직접 접근을 하면 해당 파일까지 같이 포함된다고 하더라구요. 실제로 비교했을 때도 번들 사이즈가 약간 더 큰걸 확인했습니다. 그래서 홈페이지의 용량이 크다면 분리하는게 괜찮다고 생각합니다.

❓️최적화를 위해 memo를 사용했는데, 무작정 memo를 쓰는 것은 지양해야 한다고 들었습니다. 언제 memo를 쓰면 좋은지 클린만의 기준이 있나요?

  • 사실 뜨거운 감자죠 memo를 통한 최적화. 리액트의 렌더링 최적화를 위해서는 memo를 사용하지만, 단순히 같은 컴포넌트가 여러번 호출되기 때문에 리렌더링을 방지하기 위해 사용하지는 않는것 같습니다. 사실 리액트 렌더링 자체가 비용이 큰 것은 아니라고 생각이 돼서 크게 의식을 하면서 memo를 사용하지는 않네요...ㅎ

❓️ 클로저를 사용하면서 let을 썼는데, const를 사용해서 객체 안의 프로퍼티를 바꾸는 방식과 고민했습니다. 혹시 클린은 어떤 방식을 선호하시나요? 이유도 궁금합니다!

  • 야미가 작성한 변수에는 gif의 배열을 받기 위한 변수였는데, 단일 값만 받는 경우라면 let을, 여러 값을 다루는 경우라면 const로 객체를 만들어서 프로퍼티로 만들것 같습니다. 이번 코드같은 경우에는 저라면 let을 사용할 것 같네요!

❓성능 최적화하면서 어떤 게 가장 힘들었나요?

  • LayoutShit를 잡는게 가장 힘들었던 것 같아요. 사실 처음 듣는 단어이기도 하고 브라우저 파이프라인을 제대로 알고 있지 않다보니 쉽지 않았던 것 같습니다.

❓클린이 자랑하고 싶은 부분 있나요?

  • 미천한 코드라... 크게 자랑할만한 부분은 아니지만 이번 미션을 하면서 requestAnimationFrame이라는 키워드에 대해 새롭게 알게 된 것 같아서 좋았습니다!

href="https://fonts.googleapis.com/css2?family=Josefin+Sans:ital,wght@0,400;0,700;1,400;1,700&display=swap"
rel="stylesheet"
/>
<link rel="preload" as="image" href="/static/hero.webp" />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이미지 preload 좋네요 👍 속도에 한번 감탄했습니다.!
image

하지만, Home이 아닌 다른 페이지에서 url을 치고 바로 접근했을 떄도 hero이미지를 불러오는데 자칫하면 불필요한 이미지 로드가 될수도 있는데 야미는 어떻게 생각하시나요?

Copy link
Author

@feb-dain feb-dain Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 사용자 경험을 위해 약간의 희생을 감수했습니다😂😂

만약 이미지가 중요하지 않거나, 이미지가 함께 요청돼 Search의 렌더링 속도가 많이 느려진다면 또는 분리를 해서 얻는 이점이 분리하지 않은 것보다 크다면 분리를 할 것 같습니다!

제가 확인했을 때는 위 상황 중 하나에 들어가지 않는 것 같아 preload를 진행했는데 클린은 어떻게 생각하시나요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 지역적으로 사용되는 리소스를 모든 페이지에서 부른다는게 불필요한 요청이라고 생각돼서 최대한 지양할 것 같아요. 속도 측면에서 많이 차이가 나지는 않지만, 비단 Search에서 새로고침을 해도 사용하지 않는 이미지가 불러와지는 것은 명백하니까요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 미션에서의 hero 이미지는 Home의 50% 넘게 차지하는 부분이기도 하고, preload함으로써 캐싱되어 있기에 Search에서 Home으로 왔을 때 바로 hero 이미지가 렌더링 되어 사용자 경험에서 좋다고 판단했습니다. 빠른 네트워크 환경이라면 모를까 느린 네트워크 환경에서는 화면의 50% 이상을 차지하는 이미지가 빨리 안 뜨면 불편할 거라고 생각했거든요😄

"license": "MIT",
"dependencies": {
"@giphy/js-fetch-api": "^4.1.1",
"@giphy/js-types": "^4.2.1",
"@react-icons/all-files": "^4.1.0",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webpack에서 usedExports 속성을 사용하면 자동으로 tree shaking을 해주는데 @react-icons/all-files를 사용하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음엔 그런 방법이 있는지 몰랐습니다😂

알고나서는 이미 all-files를 적용한 뒤라 다시 react-icons를 설치하고 usedExports를 사용했을 때의 이점이 없는 것 같아 그대로 적용했습니다!

src/App.css Outdated
@@ -1,3 +1,5 @@
@import url('https://fonts.googleapis.com/css2?family=Josefin+Sans:ital,wght@0,400;0,700;1,400;1,700&display=fallback');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@import 방식으로 폰트를 다운받을 경우에 불필요한 요청이 추가되고, 속도면에서도 좀 더 느린걸로 알고 있는데 @import를 통해서 폰트를 받아오신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

link 방식이 더 느리다고 잘못 알고 있었네요! 알려주셔서 감사합니다 😄😄
간단하고 더 최적화된 방법이라 선택했는데 리팩터링 해야 겠네요!

@@ -23,18 +23,39 @@ function convertResponseToModel(gifList: IGif[]): GifImageModel[] {

export const gifAPIService = {
/**
* treding gif 목록을 가져옵니다.
* trending gif 목록을 가져옵니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오타 수정 좋네요 👍

@@ -0,0 +1,3 @@
.loading {
height: 100vh;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -22,7 +22,7 @@ const Home = () => {
<img className={styles.heroImage} src={heroImage} alt="hero image" />
<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>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제목 순서 지키는거 좋습니다!

@@ -49,6 +70,11 @@ module.exports = {
]
},
optimization: {
minimize: false
minimize: true,
minimizer: ['...', new TerserPlugin(), new CssMinimizerPlugin()],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webpack v4 이상 부터는 production모드일때 자동으로 최적화를 해주는데, 거기에 TerserPlugin이 있는걸로 알고있습니다!

Copy link
Author

@feb-dain feb-dain Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞습니다!

하지만 analyzer로 확인한 결과 bundle 크기가 더 줄어들기에 적용해주었습니다. 그리고 크기가 더 줄어든 이유는 직접 설치함으로써 webpack 내장 Terser 보다 더 최신화된 Terser를 사용할 수 있어서라고 들었습니다 (아직 확인을 못해서 확실하진 않습니다)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

최신 버전이 설치된다는 것은 몰랐네요!

Copy link
Author

@feb-dain feb-dain Sep 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

직접 설치하면 가장 최신인 버전이 설치되니까요!
확인 결과, 현재 기준 webpack은 v5.3.7, 제 프로젝트에서는 v5.3.9를 사용하고 있네요. webpack은 최신 업데이트를 잘 해주니, 신경쓸 만큼 버전이 크게 달라지진 않을 것 같습니다😄그리고 지금 Terser를 지워봤는데 analyzer에서 차이가 없네요. 🤔 초반에 크기가 줄었다고 생각한 건... 뭔가 실수가 있었나봅니다

@@ -19,12 +24,28 @@ module.exports = {
devtool: 'source-map',
plugins: [
new HtmlWebpackPlugin({
template: './index.html'
template: './index.html',
minify: {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webpack에서 production 모드일 때 자동으로 true가 되는 옵션들이라고 합니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 그렇군요!! 👍

@@ -2,3 +2,5 @@ declare module '*.png';
declare module '*.jpg';
declare module '*.gif';
declare module '*.svg';
declare module '*.mp4';
declare module '*.webp';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 gif가 영상이라기 보다는 움직이는 그림 (가장 큰 차이가 소리가 없다는 거죠)에 더 가깝다고 생각해서 webp로 변경을 했었습니다.
혹시 야미는 mp4로 변환한 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 플러그인 대신 수동으로 이미지를 변환하고 압축했는데, 수동으로 webp로 변환하면 멈춰있는 이미지로 바뀌더라구요! 그래서 webm으로 바꿔봤는데, mp4로 변환/압축했을 때보다 용량이 더 커서 mp4를 선택했습니다!

@@ -37,10 +58,10 @@ module.exports = {
},
{
test: /\.css$/i,
use: ['style-loader', 'css-loader']
use: [MiniCssExtractPlugin.loader, 'css-loader']

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 dev 환경에서는 style-loader를 사용하고 prod일때 MiniCssExtractPlugin을 사용했는데, dev 환경에서도 MiniCssExtractPlugin를 사용하신 이유가 있나요?

Copy link
Author

@feb-dain feb-dain Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미션할 때 prod 모드로 진행했고 (프로덕션에서의 bundle 크기 및 네트워크 상태를 즉각적으로 확인하기 위해), prod 모드로 진행했을 때 딱히 불편함을 못 느껴서 분리하지 않았습니다

분리했을 때 장단점이 궁금해서 알아봤는데

  1. 변경된 스타일 바로 확인 가능
  2. 개발자 도구에서 디버깅 용이
  3. 빌드 시간 단축

dev에서 MiniCssExtractPlugin 대신 style-loader를 사용했을 때 얻는 이점이 꽤 있군요! 실제 프로젝트를 진행하게 된다면 분리하는 게 좋을 것 같네요 👍

그런데 미션하면서 느리다는 느낌은 받지 못해서 1, 3에서 속도가 얼마나 차이나는지 궁금하네요 ! 클린은 분리하면서 속도 차이를 체감하셨나요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빌드 시간은 상당히 차이가 많이 느껴지더라구요! (4초 vs 11초) 물론 MiniCssExtractPlugin만의 이유는 아닐거라고 생각합니다!

@feb-dain
Copy link
Author

feb-dain commented Sep 8, 2023

안녕하세요?

반갑습니다 야미 감회가 새롭군요!

코드 리뷰를 하면서 야미의 디테일함에 한번 감탄하고 갑니다.👍

몇 가지 궁금한 사항이 있어서 코멘트 남겨보겠습니다!

403 에러 발생

image

Search 페이지로 바로 접근했을 때 search 파일을 받아오지 못하고 있는데, (해당 페이지에서 새로고침 해도 같은 현상이 나타납니다) 아마 cloudFront에서 에러페이지 처리가 안되어 있는것 같아요!! 한번 확인해 보시면 좋을 것 같습니다.

헉😂 수정했습니다!

response header 정책

response header 정책으로 cache-control을 1년으로 설정하셨던데, 그렇다면 새롭게 빌드한 index.html 파일은 어떻게 업데이트를 해야 하나요? 해시 값 설정을 통한 캐시 버스팅으로 다른 파일들은 캐시에 저장되지 않는다고 해도 index.html은 이름이 같기 때문에 업데이트 되지 않을것이라고 생각이 되는데 야미의 생각이 궁금합니다!

미션을 하면서 index.html이 변경된 적이 없어 변경이 많이 없는 부분이라고 생각해 1년으로 설정했습니다! 만약 업데이트 하려면 수동으로 무효화를 해주면 됩니다!

transform

top, left 대신에 transform을 사용하신 이유가 있나요?

Top, left를 사용하면 reflow가 발생하고 transform을 사용하면 repaint만 발생합니다! 성능 최적화를 위해 transform을 사용했습니다

❓클린이 자랑하고 싶은 부분 있나요?

  • 미천한 코드라... 크게 자랑할만한 부분은 아니지만 이번 미션을 하면서 requestAnimationFrame이라는 키워드에 대해 새롭게 알게 된 것 같아서 좋았습니다!

❓️ requestAnimationFrame을 왜 사용했는지 궁금합니다! 적용하기 전에도 frame drop이나 Partially Presented Frame이 별로 발생하지 않기도 하고, 오히려 저는 적용 후에 Partially Presented Frame이 더 많이 발생하더라구요😂 클린은 그런 문제는 없었나요?

  • 혹시나 궁금해할까봐 React lazy를 Search에만 적용하는 걸로 다시 바꾼 이유는 블로그에 적혀있습니다!

Copy link

@hozzijeong hozzijeong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변경 사항 확인했습니다 고생하셨어요 야미!

@hozzijeong hozzijeong merged commit bece609 into woowacourse:feb-dain Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants