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

[Feature/BAR-2] Async Boundary 구현 #1

Merged
merged 1 commit into from Dec 10, 2023
Merged

[Feature/BAR-2] Async Boundary 구현 #1

merged 1 commit into from Dec 10, 2023

Conversation

wonjin-dev
Copy link
Member

@wonjin-dev wonjin-dev commented Dec 1, 2023

Summary

구현 내용 및 작업한 내역을 요약해서 적어주세요

  • ErrorBoundary
  • Suspense

To Reviewers

PR을 볼 때 주의깊게 봐야하거나 말하고 싶은 점을 적어주세요

  • React Query 연결 이후 추가 작업 예정 (에러 관련)
  • 폴더 구조 관련

tsconfig.json Outdated
Copy link
Member Author

@wonjin-dev wonjin-dev Dec 6, 2023

Choose a reason for hiding this comment

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

/src/* 1뎁스 폴더에 관하여 alias 작성이 필요합니다.

@dmswl98 @dongkyun-dev

Copy link
Contributor

Choose a reason for hiding this comment

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

@wonjin-dev
pages도 추가하면 안되나요?

Copy link
Member Author

@wonjin-dev wonjin-dev Dec 7, 2023

Choose a reason for hiding this comment

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

@가 root를 가리키는데 혹시 따로 필요하다고 생각하신 이유가 있을까요 ?

@/pages @pages

Copy link
Contributor

Choose a reason for hiding this comment

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

@wonjin-dev
pages 폴더도 그 위치만 밖으로 나와있을 뿐 import 경로에서 사용 형태는 동일할 것으로 생각했어서
@/pages 의 형태가 아닌 @pages 형태가 되면 어떨까 했습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

이후 케이스 추가가 필요합니다.

AsyncBoundary뿐 아니라 별도의 컴포넌트로 도 사용될 수 있게 분리

Copy link
Member Author

Choose a reason for hiding this comment

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

예시 파일.
디자인 안이 나오면 확인하여 기본 로딩 값에 대한 컴포넌트 구현이 필요합니다

import LoadingView from '../Loading';

interface AsyncBoundaryProps {
loading?: ReactNode;
Copy link
Member Author

Choose a reason for hiding this comment

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

loadingFallback라는 prop명이 더 정확한데,,, loading으로 축약하였습니다.

Copy link
Member

Choose a reason for hiding this comment

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

<ErrorBoundary>
      <Suspense fallback={}>{children}</Suspense>
</ErrorBoundary>

사용되는 곳을 고려하면, loadingFallback 또는 fallback도 나쁘지 않은 것 같아요.

const { error } = this.state;

if (error) {
return <CommonErrorScreen error={error} />;
Copy link
Member Author

Choose a reason for hiding this comment

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

loading과 마찬가지로 팩토리가 필요.
추후 추가할 예정입니다 !

Copy link
Member

@dmswl98 dmswl98 left a comment

Choose a reason for hiding this comment

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

굿굿~! 수고하셨습니당 👍🏻

type?: LoadingViewType;
}

const LoadingView: FC<LoadingViewProps> = ({ type = 'spinner' }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const LoadingView: FC<LoadingViewProps> = ({ type = 'spinner' }) => {
const LoadingView = ({ type = 'spinner' }: LoadingViewProps) => {}

React.FC는 지양하는 게 좋아서 이렇게 작성하면 어떨까요?
요기 참고하면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

우선 토픽 공유주셔서 감사해요 ! 알고 있던 토픽인데 현재는 해당하지 않는 이슈입니다.
DefinitelyTyped/DefinitelyTyped#56210

컨벤션 영역이라고 생각되어서 차주 회의에 얘기해보면 좋을것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

요약하면 React18부터는 children을 암묵적으로 가지지 않습니다.

import LoadingView from '../Loading';

interface AsyncBoundaryProps {
loading?: ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

<ErrorBoundary>
      <Suspense fallback={}>{children}</Suspense>
</ErrorBoundary>

사용되는 곳을 고려하면, loadingFallback 또는 fallback도 나쁘지 않은 것 같아요.

@@ -0,0 +1,20 @@
import { Suspense, type ReactNode, type PropsWithChildren } from 'react';
import ErrorBoundary from './components/ErrorBoundary';
import LoadingView from '../Loading';
Copy link
Member

Choose a reason for hiding this comment

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

Loading과 LoadingView 중 하나로 통일하는 건 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

Loading은 폴더명으로 도메인적 맥락을 가져갑니다!
LoadingView는 로딩 상태에 대한 뷰 컴포넌트로 팩토리 역할을 하는 컴포넌트여서 다르게 명명했습니다.

혹시 통일하는게 더 명시적일까요 ??

Copy link
Member

@dmswl98 dmswl98 left a comment

Choose a reason for hiding this comment

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

수고하셨습니당!!

Copy link
Contributor

@dongkyun-dev dongkyun-dev left a comment

Choose a reason for hiding this comment

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

수고하셧습니다!

@wonjin-dev wonjin-dev merged commit 301d537 into main Dec 10, 2023
@wonjin-dev wonjin-dev deleted the feature/BAR-2 branch December 10, 2023 15:38
@dmswl98 dmswl98 changed the title [Feature/BAR-2] Async Boundary 구현 [Feature/BR-2] Async Boundary 구현 Dec 12, 2023
@dmswl98 dmswl98 changed the title [Feature/BR-2] Async Boundary 구현 [Feature/BAR-2] Async Boundary 구현 Dec 12, 2023
@dmswl98 dmswl98 added the feature label Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants