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

[팀 서비스 클론 코딩] 황펭(양이진) 미션 제출합니다. #1

Merged
merged 20 commits into from
Oct 13, 2023

Conversation

Leejin-Yang
Copy link

@Leejin-Yang Leejin-Yang commented Oct 9, 2023

🎨 1단계 - 팀 서비스의 일부 페이지 클론 코딩

안녕하세요 우스! 마지막 미션 리뷰 잘 부탁드립니다 😊

🚀 구현한 페이지의 주소와 방식

✅ 체크리스트

선택한 렌더링 방식의 이유

  • 선택한 렌더링 방식이 어떤 페이지에 적합한지, 왜 선택했는지 이유 설명

렌더링 방식은 ISR을 사용했습니다. ISR은 빌드 타임에 정적으로 페이지를 생성하지만, 데이터의 변경이 일어날 수 있는 페이지에 적합합니다. 데이터 요청에 대해 시간을 설정한다면, 시간이 지났을 때 서버에서 사전 생성하고 업데이트합니다. 페이지를 빌드할 때 정적으로 한 번만 생성하는 것이 아니라 배포 후에도 재배포 없이 계속 업데이트 됩니다. 즉, 데이터 업데이트 시 다시 빌드하고 다시 배포하는 일을 하지 않아도 됩니다. 또한 정적 페이지로 컨텐츠가 포함되어 있어 검색 엔진 최적화에 유리하고, 매 요청마다 서버에서 렌더링을 하지 않아 많은 트래픽을 처리하는데도 유리합니다.

저는 이번에 펀잇 랜딩 페이지를 선택해 미션을 진행했습니다. 랜딩 페이지의 경우 사용자가 서비스에 접속했을 때 처음 마주하는 메인 페이지이므로 많은 사용자가 유입될 수 있습니다. 그래서 정적 페이지로 클라이언트에서 렌더링하는 자바스크립트도 줄이고, 매 요청마다 렌더링하는 것이 아닌 빌드 타임에 페이지를 미리 만들어 빠르게 제공하고자 했습니다. 그리고 랜딩 페이지에는 꿀조합, 상품, 리뷰 랭킹이 있는데, 이 데이터는 변경되는 주기(7일 또는 14일)가 있습니다. 따라서 각 요청에 revalidate 시간을 설정해 직접 배포하는 일 없이 자동으로 다시 빌드하고 배포 하도록 했습니다.

코드 재사용

  • 기존 서비스에서 구현한 리액트 컴포넌트의 재사용 여부와 활용 방식에 대해 설명

대부분 서버 컴포넌트로 재사용할 수 있었습니다. 다만 Carousel의 경우 window 객체에 접근하고 있어 쪼금 수정을 했습니다.

캐러셀은 화면 폭에 따라 너비를 가져오기 위해 window.innerWidth를 사용했습니다. 하지만 서버 환경에서는 window 객체에 접근할 수 없어 수정을 했습니다. 캐러셀 컨테이너에 ref를 걸고 useEffect를 통해 window에 resize 이벤트를 등록 후 너비를 상태에 저장했습니다(useContainerWidth). 하지만 초기값이 0이어서 랭킹 아이템이 처음에 좁아져서 나왔는데.. 이는 opacity 속성을 통해 안 보이게 했습니다(혹시 더 좋은 방법이 있을까요.?). 그렇게 window 객체 직접 접근 없이 렌더링할 수 있도록 수정했습니다.!


우스 미리 리뷰 감사드립니다 👍

@Gilpop8663 Gilpop8663 self-requested a review October 9, 2023 10:59
Copy link

@Gilpop8663 Gilpop8663 left a comment

Choose a reason for hiding this comment

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

안녕하세요 황펭 🌊🌊🌊

렌더링 방식을 ISR로 하신 이유에 대해 적어주셔서 감사합니다

이 데이터는 변경되는 주기(7일 또는 14일)가 있습니다. 
따라서 각 요청에 revalidate 시간을 설정해 직접 배포하는 일 
없이 자동으로 다시 빌드하고 배포 하도록 했습니다.

이에 대해서 궁금한 점이 생겼습니다. 제가 이해한 바로는 사이트 배포 이후 revalidate로 설정한 시간 주기가 지나면 새롭게 패치해서 사용자에게 보여주는 것으로 이해하였는데요

서버에서 랭킹이 바뀌는 주기와 사이트를 배포하여서 새롭게 패치하는 주기가 서로 다르게 될 것 같아요

예를 들어서 서버에서는 10월 10일 오전 12시에 랭킹을 업데이트를 하였다고 가정을 해봅니다.

클라이언트에서는 배포를 10월 8일 오후 4시에 하였고, 패치 주기가 지나지 않아 서버에서는 바뀌었지만 사용자에게는 보여지지 않을 것으로 예상이 됩니다.

이에 대해서 어떻게 생각하시는 지 궁금합니다


궁금하거나 새롭게 정보를 얻은 경우에 대해서 댓글로 달아보았어요!

랜딩 페이지를 Nextjs로 구현하면서 얻은 고민들을 배울 수 있어서 좋았습니다 👍👍

Comment on lines +4 to +9
remotePatterns: [
{
protocol: 'https',
hostname: process.env.NEXT_PUBLIC_IMAGE_HOSTNAME,
pathname: `${process.env.NEXT_PUBLIC_IMAGE_PATHNAME}/*`,
},

Choose a reason for hiding this comment

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

황펭 덕분에 remotePatterns에 대해 알아볼 수 있었어요 👍👍👍

Comment on lines +13 to +37
export default function RootLayout({
children,
}: {
children: React.ReactNode;
}) {
return (
<html lang='ko'>
<head>
<link
rel='stylesheet'
as='style'
crossOrigin='anonymous'
href='https://cdn.jsdelivr.net/gh/orioncactus/pretendard@v1.3.8/dist/web/variable/pretendardvariable-dynamic-subset.css'
/>
</head>
<body>
<div className='layout-container'>
<Header />
<main className='main'>{children}</main>
<NavigationBar />
</div>
</body>
</html>
);
}

Choose a reason for hiding this comment

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

레이아웃에서 html, head 과 같은 코드는 _document로 분리하는 것은 어떻게 생각하세요?

Next에서는 head도 따로 Next/Head를 사용하는 것 같아요

https://nextjs.org/docs/pages/building-your-application/routing/custom-document

예시 layout

export default function Layout({ children }) {
  return (
  <div className='layout-container'>
          <Header />
          <main className='main'>{children}</main>
          <NavigationBar />
        </div>
  )
}

Choose a reason for hiding this comment

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

그리고 nextjs에서는

<!DOCTYPE html>과 같은 코드는 필요없나 확인해보았는데요

자동으로 적용된 것을 확인할 수 있었어요

image

Choose a reason for hiding this comment

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

잘못된 지식으로 코멘트를 적었네요

RootLayout에서 html과 body 태그를 지정하고, seo에 사용되는 속성들도 head에 적으면 되네요 😅😅

위의 적었던 _document는 Page Router 방식에서만 사용하는 것으로 파악했어요!

image

https://nextjs.org/docs/app/building-your-application/routing/pages-and-layouts#root-layout-required

Copy link
Author

Choose a reason for hiding this comment

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

13부터는 서버컴포넌트에서 head안에 작성할 수 있는 옵션이 몇개 있더라구요!
직접 작성하거나 함수로 작성하는 방법 링크 남깁니다~

https://nextjs.org/docs/app/api-reference/functions/generate-metadata#metadata-fields
https://nextjs.org/docs/app/api-reference/functions/generate-metadata#generatemetadata-function

Choose a reason for hiding this comment

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

감사합니다 👍👍👍

width={160}
height={27}
alt='FUNEAT Logo'
priority

Choose a reason for hiding this comment

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

디테일이 있으시네요!

priority를 사용하는 기준이 있으실까요? 어느 상황에서 사용하는 지 궁금합니다

Copy link
Author

Choose a reason for hiding this comment

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

중요한 이미지거나 이미지 용량이 커 응답 속도가 느릴때 미리 가져오도록 했습니다!

}: SvgIconProps) => {
return (
<svg width={width} height={height} fill={color} {...props}>
<use xlinkHref={`/sprite.svg#${variant}`} />

Choose a reason for hiding this comment

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

use 태그가 있는 것을 처음 알게 되었어요

xlinkHref는 더 이상 권장되지 않는 속성이라고 하네요

https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href

Comment on lines 22 to 38
{image ? (
<Image
src={image}
className={styles.recipeImage}
width={60}
height={60}
alt={`${rank}위 이미지`}
/>
) : (
<Image
src='/plate.svg'
className={styles.recipeImage}
width={60}
height={60}
alt={`${rank}위 이미지`}
/>
)}

Choose a reason for hiding this comment

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

아래와 같이 사용하는 것은 어떠세요?

        <Image
          src={image ?? '/plate.svg'}
          className={styles.recipeImage}
          width={60}
          height={60}
          alt={`${rank}위 이미지`}
        />

Copy link
Author

Choose a reason for hiding this comment

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

오! 감사합니다 ㅎㅎ

recipes: RecipeRanking[];
}

const RecipeRanking = async ({ recipes }: RecipeRankingProps) => {

Choose a reason for hiding this comment

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

RecipeRanking 컴포넌트에서만 async 처리를 해주었는데 이유가 궁금합니다

Copy link
Author

Choose a reason for hiding this comment

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

안에서 데이터를 요청하는 방식으로 작성했다가 수정했는데 놓쳤네요 😅

@@ -0,0 +1,17 @@
'use client';

Choose a reason for hiding this comment

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

제가 use client에 대해 잘 이해를 못했는데요! React 관련된 메서드를 사용할 때 use client를 붙히는 것으로 이해하면 될까요?

https://nextjs.org/docs/app/building-your-application/rendering/client-components

Copy link
Author

Choose a reason for hiding this comment

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

옙 맞습니다 ㅎㅎ
useState, useEffect와 같이 클라이언트 사이드에서 사용하는 훅이나 브라우저 환경에서만 접근할 수 있는 값들을 사용할 수 있어요

export const getRecipeRanking = async () => {
const endpoint = `${ENDPOINT}/recipes`;
return fetchApi<RecipeRankingResponse>(endpoint, {
next: { revalidate: 7 * 24 * 60 * 60 },

Choose a reason for hiding this comment

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

처음 보는 사용 방법이라 무엇인가 했더니 Next에서만 사용하는 속성이였군요! 덕분에 next/fetch에 대해 공부할 수 있었습니다 👍👍👍

interface NextFetchRequestConfig {
  revalidate?: number | false
  tags?: string[]
}

interface RequestInit {
  next?: NextFetchRequestConfig | undefined
}

Copy link
Author

Choose a reason for hiding this comment

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

Next가 fetch를 한번 래핑해서 fetch만으로 여러 동작을 할 수 있어 편하더라구요!

Comment on lines +10 to +21
const Error = ({ error, reset }: ErrorProps) => {
const errorMessage = error.message ?? '알 수 없는 오류가 발생했습니다.';

return (
<section className={styles.error}>
<h2 className={styles.title}>{errorMessage}</h2>
<button type='button' onClick={reset} className={styles.resetButton}>
다시 시도하기
</button>
</section>
);
};

Choose a reason for hiding this comment

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

Next에서는 error 파일을 만들면 에러 바운더리 역할을 해주네요!? 너무 편한데요 🔥🔥🔥

https://nextjs.org/docs/app/api-reference/file-conventions/error

Comment on lines +1 to +29
import { RefObject, useEffect, useRef, useState } from 'react';

export const useContainerWidth = (ref: RefObject<HTMLDivElement>) => {
const [width, setWidth] = useState(0);
const rafId = useRef<number | null>(null);

useEffect(() => {
const handleResize = () => {
if (ref.current) {
setWidth(ref.current.offsetWidth);
}

rafId.current = requestAnimationFrame(handleResize);
};

handleResize();
window.addEventListener('resize', handleResize);

return () => {
if (rafId.current) {
cancelAnimationFrame(rafId.current);
}

window.removeEventListener('resize', handleResize);
};
}, []);

return width;
};

Choose a reason for hiding this comment

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

ref로 충분히 잘 해결하셨다고 생각이 들어요

굳이 바꿔보자면 useEffect에서는 window가 가능하더라고요

이렇게 했을때는 ref를 인자로 안받아도 되는 이점이 있는 것 같아요

export const useContainerWidth = () => {
  const [width, setWidth] = useState(0);
  const rafId = useRef<number | null>(null);

  useEffect(() => {
    const handleResize = () => {
      setWidth(window.innerWidth);

      rafId.current = requestAnimationFrame(handleResize);
    };

    handleResize();
    window.addEventListener('resize', handleResize);

    return () => {
      if (rafId.current) {
        cancelAnimationFrame(rafId.current);
      }

      window.removeEventListener('resize', handleResize);
    };
  }, []);

  return width;

Copy link
Author

Choose a reason for hiding this comment

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

스크린샷 2023-10-13 오후 4 23 36

우스가 작성한게 기존 로직이랑 비슷해서 작성해봤는데 화면폭을 따라서 컨테이너보다 넓어지는거 같더라구요.? 🥲
좋은 의견 감사해요 우스 👍

Choose a reason for hiding this comment

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

의도치 않은 동작이 발생하네요 😅 알려주셔서 감사합니다

@Leejin-Yang
Copy link
Author

우스 섬세한 리뷰 감사해요 😊

서버에서 랭킹이 바뀌는 주기와 사이트를 배포하여서 새롭게 패치하는 주기가 서로 다르게 될 것 같아요

우스의 말을 들어보니 저도 생각 못했던 부분이네요.!
현재는 프론트와 백이 같이 운영에 배포되기 때문에 문제가 없다고 생각했어요. 만약 배포가 따로 이루어진다면 ISR 주기를 줄인다던가, SSR을 사용해 데이터 검증을 계속 해볼 수 있을것 같습니다! 좋은 의견 감사해요 우스


그 외 코멘트 달아준 부분에 의견 남기고 피드백 반영했는데 확인 부탁드려요 👍

@Gilpop8663
Copy link

황펭 코드 리뷰 반영된 것 확인하였습니다 👍

서버 주기 관련

서버에서 빌드된 시간으로 계산해서 하는군요! 잘 몰랐네요 저는 몇월 몇일과 같이 시간을 기준으로 업데이트 하는 것이라고 생각했었어요

서버와 프론트의 데이터 패칭 주기가 같으니 현재 선택하신 ISR 방식이 가장 적절해보이네요 💯💯💯

어프로브 하겠습니다 2단계에서 만나요!

@Gilpop8663 Gilpop8663 merged commit c816650 into woowacourse:leejin-yang Oct 13, 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