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

[12팀 하정원] 프레임워크 없이 SPA 만들기 #16

Closed
wants to merge 33 commits into from

Conversation

JayeHa
Copy link

@JayeHa JayeHa commented Sep 24, 2024

과제 체크포인트

기본과제

1) 라우팅 구현:

  • History API를 사용하여 SPA 라우터 구현
    • '/' (홈 페이지)
    • '/profile' (프로필 페이지)
    • '/404' (Not Found 페이지)
  • 각 라우트에 해당하는 컴포넌트 렌더링 함수 작성
  • 네비게이션 이벤트 처리 (링크 클릭 시 페이지 전환)

2) 사용자 관리 기능:

  • LocalStorage를 사용한 간단한 사용자 데이터 관리
    • 사용자 정보 저장 (이름, 간단한 소개)
    • 로그인 상태 관리 (로그인/로그아웃 토글)
  • 로그인 폼 구현
    • 사용자 이름 입력 및 검증
    • 로그인 버튼 클릭 시 LocalStorage에 사용자 정보 저장
  • 로그아웃 기능 구현
    • 로그아웃 버튼 클릭 시 LocalStorage에서 사용자 정보 제거

3) 프로필 페이지 구현:

  • 현재 로그인한 사용자의 정보 표시
    • 사용자 이름
    • 간단한 소개
  • 프로필 수정 기능
    • 사용자 소개 텍스트 수정 가능
    • 수정된 정보 LocalStorage에 저장

4) 컴포넌트 기반 구조 설계:

  • 재사용 가능한 컴포넌트 작성
    • Header 컴포넌트
    • Footer 컴포넌트
  • 페이지별 컴포넌트 작성
    • HomePage 컴포넌트
    • ProfilePage 컴포넌트
    • NotFoundPage 컴포넌트

5) 상태 관리 초기 구현:

  • 간단한 상태 관리 시스템 설계
    • 전역 상태 객체 생성 (예: 현재 로그인한 사용자 정보)
  • 상태 변경 함수 구현
    • 상태 업데이트 시 관련 컴포넌트 리렌더링

6) 이벤트 처리 및 DOM 조작:

  • 사용자 입력 처리 (로그인 폼, 프로필 수정 등)
  • 동적 컨텐츠 렌더링 (사용자 정보 표시, 페이지 전환 등)

7) 기본적인 에러 처리:

  • 잘못된 라우트 접근 시 404 페이지로 리다이렉션
  • 로그인 실패 시 에러 메시지 표시

심화과제

1) 고급 라우팅

  • 라우트 가드 구현
    • 로그인 상태에 따른 접근 제어
    • 비로그인 사용자의 특정 페이지 접근 시 로그인 페이지로 리다이렉션

2) 고급 이벤트 처리

  • 이벤트 위임 활용

리뷰 받고 싶은 내용

혹시 제 코드를 보시면서 궁금해하실 것 같은 부분이나, 제가 코드를 작성하면서 나름대로 의도한 바가 있는 코드에 셀프리뷰를 달았습니다!

리뷰어님께서는 이에 대해서 어떤 의견을 가지고 계신지 궁금하고, 그 외에도 혹시 제가 부족한 부분이나 놓쳤던 부분에 대해서 알려주시면 너무너무 감사드릴 것 같습니당 (매운맛으로 막 무자비하게 해주셔도 되용 🥰)

궁금한 점

프로젝트 폴더구조를 어떻게 가져가야할지 궁금해요!

일단 지금 프로젝트의 폴더구조는 아래와 같이 구성했는데요,
image

만약, 각 페이지단위에서만 쓰는 컴포넌트가 생긴다면,
components/login components/profile 처럼 페이지나 도메인별로 디렉토리를 만들어서 구분하는게 관리하는게 좋을 것 같은데요.

그런데 만약에 전역적으로 사용되는게 아니라, 연관있는 도메인을 가진 페이지에서만 쓰이는 컴포넌트 ex) 로그인 페이지와 프로필 페이지에서만 쓰이는 컴포넌트 가 생기면, 이런것들은 어떻게 분류해야 좋을까요? 뭔가 shared로 넣기에는 왠지모르게 찜찜한 느낌이 들어요.

과제를 수행하면서 느낀점

과제 시작 전 생각

"과제 빨리 다 하고 함수 컴포넌트로도 만들어봐야징~😙"

과제 제출 후 생각

image

기타

과제 난이도

4/5
첫 주차 과제이지만 절대 쉽지 않은 과제였습니다. 그래서 더 좋았어요!

@JayeHa JayeHa changed the title [12조/하정원] 프레임워크 없이 SPA 과제 만들기 (WIP) [12팀 하정원] 프레임워크 없이 SPA 과제 만들기 (WIP) Sep 24, 2024
@JayeHa JayeHa changed the title [12팀 하정원] 프레임워크 없이 SPA 과제 만들기 (WIP) [12팀 하정원] 프레임워크 없이 SPA 과제 만들기 Sep 24, 2024
@JayeHa JayeHa changed the title [12팀 하정원] 프레임워크 없이 SPA 과제 만들기 [12팀 하정원] 프레임워크 없이 SPA 과제 만들기 (WIP) Sep 24, 2024
@JayeHa JayeHa changed the title [12팀 하정원] 프레임워크 없이 SPA 과제 만들기 (WIP) [12팀 하정원] 프레임워크 없이 SPA 만들기 (WIP) Sep 24, 2024
@JayeHa JayeHa changed the title [12팀 하정원] 프레임워크 없이 SPA 만들기 (WIP) [12팀 하정원] 프레임워크 없이 SPA 만들기 - 심화과제, 클래스 (WIP) Sep 24, 2024
@JayeHa JayeHa changed the title [12팀 하정원] 프레임워크 없이 SPA 만들기 - 심화과제, 클래스 (WIP) [12팀 하정원] 프레임워크 없이 SPA 만들기 - 심화과제 클래스 (WIP) Sep 24, 2024
@JayeHa JayeHa marked this pull request as draft September 24, 2024 17:04
@JayeHa JayeHa force-pushed the advenced branch 2 times, most recently from b80e1b5 to 607613e Compare September 25, 2024 16:53
Copy link
Author

@JayeHa JayeHa left a comment

Choose a reason for hiding this comment

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

혹시 제 코드를 보시면서 궁금해하실 것 같은 부분이나, 제가 코드를 작성하면서 나름대로 의도한 바가 있는 코드에 셀프리뷰를 달았습니다!

리뷰어님께서는 이에 대해서 어떤 의견을 가지고 계신지 궁금하고, 그 외에도 혹시 제가 부족한 부분이나 놓쳤던 부분에 대해서 알려주시면 너무너무 감사드릴 것 같습니당 (매운맛으로 막 무자비하게 해주셔도 되용 🥰)

Comment on lines +1 to +11
{
"printWidth": 80,
"tabWidth": 2,
"useTabs": false,
"semi": true,
"singleQuote": true,
"trailingComma": "es5",
"bracketSpacing": true,
"jsxBracketSameLine": false,
"arrowParens": "always"
}
Copy link
Author

Choose a reason for hiding this comment

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

pritter는 제 vscode의 설정과 다른 부분이 있어서 기본값들도 명시적으로 설정했습니다 :)

Comment on lines +11 to +21
init(routes: Routes) {
if (!routes['/404']) {
throw new Error("'/404' 경로가 필요합니다.");
}

this.#routes = routes;

this.#handlePopState();
window.addEventListener('popstate', this.#handlePopState.bind(this));
this.#setupLinkNavigation();
}
Copy link
Author

@JayeHa JayeHa Sep 25, 2024

Choose a reason for hiding this comment

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

router를 생성자에서 초기화하는 대신, init 메소드에서 초기화하도록 구현했습니다.

처음에는 생성자에서 초기화를 시도했지만, 페이지 이동 시 해당 인스턴스를 통해 navigateTo 같은 메소드를 사용해야 했기 때문에 init 메소드를 따로 만들었어요.

또한, navigateTostatic 메소드로 만들 수도 있을까 생각해봤지만, 페이지를 이동할 때마다 this.#routes 값을 사용해야 하므로 현재 방식이 더 적합하다고 판단했습니다! 😄

Comment on lines +37 to +46
#setupLinkNavigation() {
document.addEventListener('click', (e) => {
const target = e.target as HTMLElement;
if (target.tagName === 'A') {
e.preventDefault();
const target = e.target as HTMLAnchorElement;
router.navigateTo(target.pathname);
}
});
}
Copy link
Author

@JayeHa JayeHa Sep 25, 2024

Choose a reason for hiding this comment

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

a태그에 대한 이벤트바인딩을 전역적으로 해주었습니다.

리액트의 <Link /> 컴포넌트처럼 만들까 고민하긴 했는데,
클래스 컴포넌트는 만들 때마다 dom을 지정해줘야 하는데 a태그는 매번 그렇게 생성하기에는 너무 자주 사용한다고 생각해서 전역으로 해주었어요.

현재 요구사항에서는 이렇게 전역적으로 라우팅을 설정해도 문제는 없을 것 같다고 판단했고, 만약 나중에 외부로 링크를 보내야 하는 상황이 발생한다면 그때 처리하면 될 것 같아서, 지금으로서는 가장 효율적이라고 생각하는 방법을 선택했습니다. 😄

Comment on lines +12 to +14
if (!routes['/404']) {
throw new Error("'/404' 경로가 필요합니다.");
}
Copy link
Author

@JayeHa JayeHa Sep 25, 2024

Choose a reason for hiding this comment

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

유효하지 않은 URL일 때 반드시 오류 페이지를 보여줘야 하기 때문에, 404 경로가 설정되지 않으면 Router를 초기화할 때 에러를 던지도록 구현했습니다.

처음에는 기본값으로 404 페이지를 제공할까 고민했지만, 그렇게 하면 제가 커스텀으로 만든 컴포넌트와 결합도가 높아질 것 같아서 현재 방식으로 구현했어요.

그런데 막상 사용해보니, 어차피 제네릭을 사용하지 않고 BaseComponent만 받도록 타입을 설정해서, 그냥 기본값으로 제공해도 괜찮을 것 같다는 생각도 드네요... 😅

Comment on lines +16 to +20
private checkAccess() {
if (isLoggedIn()) {
router.navigateTo('/');
}
}
Copy link
Author

@JayeHa JayeHa Sep 25, 2024

Choose a reason for hiding this comment

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

❓ 라우트 가드를 어디에서 처리하는 것이 좋을까요?

  • 각 컴포넌트에서 처리하는 것이 좋은지
  • Router.ts에서 처리하는 것이 좋은지

현재는 라우트 가드를 각 컴포넌트에서 처리하고 있어요. 페이지마다 리다이렉트할 경로가 다르기 때문에, 컴포넌트에서 조건에 따라 해당 페이지로 이동하게끔 구현했어요.

그런데 셀프 리뷰를 하다 보니, 라우트 가드를 Router.ts에서 처리하는 것이 더 효율적일 수도 있겠다는 생각이 들었어요. 그렇게 하면 조건을 만족하지 않는 경우 아예 페이지를 렌더링하지 않고 차단할 수 있고, 라우트 관련 로직도 한 곳에서 관리할 수 있어서 코드의 일관성을 유지할 수 있을 것 같아요.

일괄적으로 라우트 관리를 한 곳에서 처리하는 것이 유지보수 측면에서도 더 편리할 것 같다는 생각이 드네요. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

전에 정원님이 리뷰 남겨주신 코드를 봤던것 같은데 그대로 적용해보면, React Router를 적용할 때 우리가 가딩을 어떻게 처리하는지 고려해보면 좋을것 같아요!
대신 우리는 프로젝트 내에서 단일 라우팅이기 때문에 해당 로직들이 포함되어도 괜찮지 않을까~정도 인 느낌입니다.
만약 확장을 고려하고 이 코드 부분이 여러 프로젝트, 여러 경로에서 사용되어야 하는 구조라면 외부에서 정의되는게 맞을것 같네여!

Copy link
Author

Choose a reason for hiding this comment

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

회사 프로젝트에서 해당 페이지에서 리다이렉트를 시키는 코드가 많다 보니, 습관적으로 이렇게 구현하게 된 것 같아요. 하지만 셀프 리뷰를 하면서 제 3자 입장에서 보니 조금 더 객관적으로 볼 수 있더라고요.

오프 코치님 말씀처럼 React Router를 참고해서 외부에서 정의하고, PrivateRoute를 구현하는 연습을 해보겠습니다.
말씀해주셔서 감사합니다! 😍😍😍

});
}

abstract template(): string;
Copy link
Author

@JayeHa JayeHa Sep 25, 2024

Choose a reason for hiding this comment

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

각 자식 컴포넌트가 자신에 맞는 템플릿을 구현해야 하기 때문에, 해당 메소드를 추상 메서드로 정의했습니다.

protected $root: Element | null;
protected state: S | undefined;

constructor(selector: string, initialState?: S) {
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
constructor(selector: string, initialState?: S) {
constructor(props: {selector: string, initialState?: S}) {

리액트처럼 이렇게 구현하는 게 더 나은 것 같기도 하고..

Comment on lines +21 to +31
private getLinkList(): Link[] {
const commonLinks = [{ text: '홈', link: '/' }];
const authLinks = isLoggedIn()
? [
{ text: '프로필', link: '/profile' },
{ text: '로그아웃', link: '#', id: LOGOUT_ID },
]
: [{ text: '로그인', link: '/login' }];

return [...commonLinks, ...authLinks];
}
Copy link
Author

@JayeHa JayeHa Sep 26, 2024

Choose a reason for hiding this comment

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

❓이 부분 괜찮은게 맞을까요?

한 눈에 알아보기 조금 힘든거 같기도 하고.. 괜찮은거 같기도 하고.. 긴가민가합니당..;ㅂ;

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
private getLinkList(): Link[] {
const commonLinks = [{ text: '홈', link: '/' }];
const authLinks = isLoggedIn()
? [
{ text: '프로필', link: '/profile' },
{ text: '로그아웃', link: '#', id: LOGOUT_ID },
]
: [{ text: '로그인', link: '/login' }];
return [...commonLinks, ...authLinks];
}
private getLinkList(): Link[] {
const commonLinks = [{ text: '홈', link: '/' }];
if(isLoggedIn()) {
return [...commonLinks, { text: '프로필', link: '/profile' }, { text: '로그아웃', link: '#', id: LOGOUT_ID }];
}
return [...commonLinks, { text: '로그인', link: '/login' }]
}

취향차이겠지만, 제 취향에는 요게 좀 더 맞스빈다 ㅎㅎ

Copy link
Author

@JayeHa JayeHa Sep 29, 2024

Choose a reason for hiding this comment

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

와, 코드를 이렇게 작성할 수도 있군요! 감사합니다! 코치님 덕분에 정말 많이 배워요. 😆

Comment on lines +33 to +35
private renderLinkList() {
return this.state
?.map(({ id, link, text }) => {
Copy link
Author

@JayeHa JayeHa Sep 26, 2024

Choose a reason for hiding this comment

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

Suggested change
private renderLinkList() {
return this.state
?.map(({ id, link, text }) => {
private renderLinkList() {
return this.state.links
?.map(({ id, link, text }) => {

여기서 state가 하나밖에 없어서 간단하게 구현하려고 데이터를 바로 넣었는데, 이름이 없으니까 헷갈리는 것 같아서 이렇게 객체로 넣는게 좋을 것 같다는 생각이 들었어요

Comment on lines +48 to +66
private handleFormSubmit(e: SubmitEvent) {
e.preventDefault();

const $nameInput = this.getElement<HTMLInputElement>(`#${ID.USER_NAME}`)!;
const $emailInput = this.getElement<HTMLInputElement>(`#${ID.EMAIL}`)!;
const $bioInput = this.getElement<HTMLInputElement>(`#${ID.BIO}`)!;

const username = $nameInput.value.trim();
const email = $emailInput.value.trim();
const bio = $bioInput.value.trim();

const userInfo: UserInfo = {
username,
email,
bio,
};

setUserInfo(userInfo);
}
Copy link
Author

@JayeHa JayeHa Sep 26, 2024

Choose a reason for hiding this comment

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

지금 살펴보니 이 부분에서 굳이 모든 input DOM 요소를 직접 가져올 필요가 없을 것 같아요.

  • form 태그의 submit 이벤트에서 각 input 값들을 가져오는 방법이 있었던 것 같고,
  • 리액트처럼 state로 하면 setState도 활용해볼 수도 있으니까 이 방법을 적용해보는 것도 정말 괜찮을 것 같단 생각이 들었습니당

Choose a reason for hiding this comment

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

const { username, email, bio } = e.target.elements
로 가져와
username.value 형태로 값을 사용하는 방법이 있을꺼같아요!

Copy link
Author

@JayeHa JayeHa Sep 27, 2024

Choose a reason for hiding this comment

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

오, 좋은 방법 공유해주셔서 감사드려요! ><

Copy link

@po4tion po4tion left a comment

Choose a reason for hiding this comment

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

1팀 김동규입니다! 코드 너무 잘 봤습니다!

const $nameInput = this.getElement<HTMLInputElement>(`#${ID.USER_NAME}`)!;
const username = $nameInput.value.trim();

if (username) {
Copy link

Choose a reason for hiding this comment

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

username의 type은 string으로 확인되는데요. 그렇다면 null 또는 undefined를 고려하지 않아도 괜찮아보이는데요. 조건문을 단순히 username만 넣는 것 보다는, username의 길이를 확인하는 직관적인 조건을 넣어주는 것은 어떻게 생각하시나요?

Copy link
Author

@JayeHa JayeHa Sep 26, 2024

Choose a reason for hiding this comment

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

안녕하세요, 동규님!
코드리뷰 정말 감사드립니다 😊

동규님께서 제안해주신 내용을 보니 이렇게 개선해주신 것 같아요.

기존 코드

if (username) {
      login({ username });
    }

동규님께서 제안해주신 코드

if (username.length > 0) {
      login({ username });
    }

동규님의 의견이 너무 좋은 의견이라고 생각합니다!

다만, 제 의견을 말씀드리면, 저에게는 오히려 전자의 코드가 "사용자 이름이 있으면 로그인을 시도해!"라는 의미로 좀 더 간결하고 자연스럽게 읽히는 것 같아요.

후자는 "이름의 길이가 0보다 길면 로그인을 해!"라는 의미라서, 개인적으로는 전자가 좀 더 직관적이지 않나 생각했어요 🤔

그리고 동규님께서 말씀해주셨던대로 username의 타입이 string이 확실하지만, 그렇다고 해서 null이나 undefined를 조건에서 꼭 제외시켜야 할까? 라는 생각이 들기도 해요.

물론 명확하게 길이 조건을 확인해주는 것도 좋은 방법이라고 생각해요! 이에 대한 다른 분들의 의견도 궁금하네요~!

다시 한번 꼼꼼한 리뷰 감사드리고, 앞으로도 좋은 의견 부탁드립니다 😄

Copy link

Choose a reason for hiding this comment

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

정원님의 소중한 의견 감사해요!
제가 제시드렸던 조건은 이런 의미를 내포하고 있다고 생각하는데요.

"어? username의 길이를 판별하는 구나! 그렇다면 username의 타입은 문자열이겠네. 0보다 커야 통과한다고? 그렇다면 한글자라도 입력이 되어야 하겠구나!"

저는 이 방법을 통해 읽는 이가 정말 필요한 정보를 단편적인 로직을 통해 전달해 줄 수 있다고 생각해요. 사용자의 이름이 있는 것 뿐만 아니라 이름의 타입에 대한 정보까지 보여줄 수 있는거죠. 타입을 미리 보여줄 수 있다면 코드를 읽을 때 "기웃거림" 현상이 줄어든다고 생각해요.

제가 생각하는 "기웃거림"이란 코드를 읽는 이가 로직을 이해하기 위해 마우스를 이동하거나, 커서를 이동하거나, 스크롤을 이동하거나, 파일을 이동하거나, 검색을 하거나, 커밋을 다시 찾아본다거나 등의 행위를 뜻해요.

여기까지는 제가 조건문을 어떻게 바라보는지에 대한 의견이었어요. 정원님의 의견을 충분히 존중해요. "이렇게 생각하는 개발자도 있구나" 정도로 봐주시면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

동규님 말씀 들어보니까, 저 조금 설득될 것 같아요 🤣🤣

사실 username이라는 변수명만 봐도 문자열이라는 것을 알 수 있다고 생각하기 때문에, 어느 쪽을 선택해도 나쁜 선택은 아니라고 생각해요.

하지만 이 과정에서 동규님께서 작은 부분까지 깊이 고민하시는 분이라는 게 느껴졌습니다.

덕분에 '다른 개발자들은 이렇게 생각할 수도 있구나' 하고 제 사고가 좀 더 유연해진 것 같아요!

제 PR에 이렇게 좋은 의견 남겨주셔서 감사드립니다. 정말 많은 도움이 되었어요 :)

const $loginForm = this.$root?.querySelector<HTMLFormElement>(
`#${ID.LOGIN_FORM}`
);
if (!$loginForm) return;
Copy link

@po4tion po4tion Sep 26, 2024

Choose a reason for hiding this comment

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

저는 if문의 조건문을 사용할 때 논리적 NOT 연산자(!)를 최대한 회피하려고 하는데요. 이유는 개발자가 조건을 읽을 때, 2번 생각해야 하기 때문이에요. loginForm의 기본 상태를 한 번 생각해보고, 그 값을 뒤집는 반대 상태를 다시 한 번 생각해봐야 하는데요.
개발자가 코드를 읽을 때, 생각을 한 번이라도 줄여줄 수 있는 로직에 큰 매력이 있다고 생각해요(개발자의 뇌를 가볍게 만들어줍니다).

Suggested change
if (!$loginForm) return;
if ($loginForm == null) return;

이런식으로 엄격한 동등 연산자가 아닌 동등 연산자를 활용하면 쉽게 구현할 수 있습니다. 정원님은 제 의견에 대해 어떻게 생각하고 계신지 궁금합니다!

Copy link
Author

@JayeHa JayeHa Sep 26, 2024

Choose a reason for hiding this comment

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

와! 정말 생각해보지 못한 부분이었는데, 좋은 리뷰 감사드립니다 :)

저도 동규님 의견에 공감해요. 같은 결과를 얻을 수 있는 코드라면, 다른 개발자가 더 직관적으로 이해할 수 있는 방식으로 작성하는 것이 훨씬 더 좋다고 생각해요.

말씀하신 부분 중에서 특히 ! 연산자가 기본 상태를 연상시킬 수 있다는 점이 저에게도 새로운 관점이었어요. 그 점을 고려하면 동규님 제안이 더 명확해 보이네요.

이 부분은 내일 꼭 코드에 반영하도록 하겠습니다! 다시 한 번 감사드려요 😆

@@ -0,0 +1,61 @@
export abstract class BaseComponent<S = unknown> {
protected $root: Element | null;
Copy link

Choose a reason for hiding this comment

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

$root가 querySelector의 반환 타입만 가진다면 JavaScript가 내부적으로 제공해주는 타입을 활용하는게 추후 유지보수 측면에서 더 좋을 것 같다는 생각을 했는데요. querySelector의 타입이 어느 순간에 휙 바뀌어 버릴수가 있는데, 그렇게 된다면 개발자가 일일이 찾아서 변경해줘야 할 것 같아요.

Suggested change
protected $root: Element | null;
protected $root: ReturnType<ParentNode['querySelector']>;

이런식으로 자바스크립트가 제공해주는 타입에 의존한다면, 개발자는 자바스크립트 변경사항과는 무관하게 타입을 수정할 필요가 없어질 것 같아요.

정원님은 어떻게 생각하시는지 의견을 듣고 싶어요!

Copy link
Author

@JayeHa JayeHa Sep 26, 2024

Choose a reason for hiding this comment

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

너무 좋은 의견이예요!

그런데 저는 동규님께서 바로 아래 코맨트에서 남겨주신 '타입을 공통화'하는 방법이 더 마음에 들어요 ㅎㅎ

Suggested change
protected $root: Element | null;
protected $root: RootElement;

이렇게요! 😆

왜냐하면 유틸리티 타입에 인터페이스를 중첩해서 그 인터페이스의 프로퍼티로 접근하는 것이 어쩌면 조금은 불필요하게 복잡해 보일 수 있을 것 같았어요.

그래서 저는 동규님께서 다른 코멘트에서 제안해주신 것처럼, 타입을 공통화하는 방식이 더 좋아보이는데, 이에 대해서 동규님은 어떻게 생각하시나요?

여러 가지 방법으로 제안해주신 덕분에 여러가지 측면에서 생각해보게 되었어요.
정말 감사합니다!! 🙇‍♀️🙇‍♀️

this.bindGlobalErrorHandling();
}

private getRootElement(selector: string): Element | null {
Copy link

Choose a reason for hiding this comment

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

getRootElement의 반환 타입은 $root가 갖는 타입과 일치할 수 밖에 없는데요. 만약에 $root 타입이 변경된다면 getRootElement도 따라서 변경해줘야 할 것 같아요.
BaseComponent.ts 파일 내에서만 사용되는 타입으로 보이니, 해당 파일 내에서 하나의 타입으로 공통화해서 사용하는 건 어떻게 생각하시나요?

Copy link
Author

@JayeHa JayeHa Sep 26, 2024

Choose a reason for hiding this comment

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

정말 좋은 의견이라고 생각해요! 훨씬 깔끔해질 것 같아요.
이 부분 꼭 반영하도록 할게용 👍

좋은 피드백 감사드립니당!! 🙇‍♀️🙇‍♀️

@JayeHa JayeHa changed the title [12팀 하정원] 프레임워크 없이 SPA 만들기 - 심화과제 클래스 (WIP) [12팀 하정원] 프레임워크 없이 SPA 만들기 - 심화과제 클래스 Sep 26, 2024
@JayeHa JayeHa changed the title [12팀 하정원] 프레임워크 없이 SPA 만들기 - 심화과제 클래스 [12팀 하정원] 프레임워크 없이 SPA 만들기 Sep 26, 2024
@JayeHa JayeHa marked this pull request as ready for review September 26, 2024 16:35
@JunilHwang JunilHwang closed this Sep 27, 2024
Copy link
Member

@jung-han jung-han left a comment

Choose a reason for hiding this comment

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

정원님 다른 분들 리뷰 남겨주신 의견 너무 좋았어요 👍

그런데 만약에 전역적으로 사용되는게 아니라, 연관있는 도메인을 가진 페이지에서만 쓰이는 컴포넌트 ex) 로그인 페이지와 프로필 페이지에서만 쓰이는 컴포넌트 가 생기면, 이런것들은 어떻게 분류해야 좋을까요? 뭔가 shared로 넣기에는 왠지모르게 찜찜한 느낌이 들어요.

pages로 분류를 해뒀다면 해당 페이지에서만 사용하는 컴포넌트는 페이지 하위에 components 를 만들어서 같은 위치에 둬도 될 것 같구요! 여러 페이지를 넘나드는 컴포넌트들에 대해서 shared에서 관리해주면 좋지 않을까요?!

Copy link
Member

@jung-han jung-han left a comment

Choose a reason for hiding this comment

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

전반적으로 정리도 깔끔하게 해주셨고 구현도 잘되어있는 것 같습니다.
스토어 부분만 조금 더 살펴봐주시면 좋을것 같아요!

@@ -17,6 +17,7 @@
"@testing-library/user-event": "^14.5.2",
"@vitest/ui": "^2.1.1",
"jsdom": "^25.0.0",
"typescript": "^5.6.2",
Copy link
Member

Choose a reason for hiding this comment

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

오~

Comment on lines +16 to +20
private checkAccess() {
if (isLoggedIn()) {
router.navigateTo('/');
}
}
Copy link
Member

Choose a reason for hiding this comment

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

전에 정원님이 리뷰 남겨주신 코드를 봤던것 같은데 그대로 적용해보면, React Router를 적용할 때 우리가 가딩을 어떻게 처리하는지 고려해보면 좋을것 같아요!
대신 우리는 프로젝트 내에서 단일 라우팅이기 때문에 해당 로직들이 포함되어도 괜찮지 않을까~정도 인 느낌입니다.
만약 확장을 고려하고 이 코드 부분이 여러 프로젝트, 여러 경로에서 사용되어야 하는 구조라면 외부에서 정의되는게 맞을것 같네여!

@@ -0,0 +1,43 @@
import { router } from './Router';
Copy link
Member

Choose a reason for hiding this comment

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

요런 파일들은 util이라기 보다는 별도의 이름이 있으면 좋겠네요 ~

Copy link
Author

Choose a reason for hiding this comment

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

코치님 말씀대로 router는 lib나 app과 같은 별도의 디렉토리를 만들어 이동시키는 게 더 좋을 것 같아요!
말씀해주셔서 감사합니다! 🥰

@@ -0,0 +1,43 @@
import { router } from './Router';
Copy link
Member

Choose a reason for hiding this comment

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

지금은 유저에 관한 로직들을 세세하게 등록해주고 있는데요!
좀 더 다양한 곳에서 쓰일 수 있는,
상태가 변경되었을 때 렌더링이 자동으로 트리거 되거나 등록한 함수들이 실행되도록 구성해보면 어떤가요?
요런 기능들을 범용적으로 다룰 수 있는 반응형 로직같은걸 어떻게 짜볼지 고민해봐도 좋을것 같아요 ㅎㅎ
Proxy 객체를 활용한다라거나..일반 객체로도 할 수 있구요!

Copy link
Author

@JayeHa JayeHa Sep 29, 2024

Choose a reason for hiding this comment

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

상태 관리를 구현하는 것이 조금 어렵게 느껴져서 마지막 순서로 미루다 보니, 어느새 과제 제출 시간이 다 되어 결국 해결하지 못한 상태로 제출하게 되었어요. 조금 부끄럽습니다 😂

Proxy를 사용하는 방법은 전혀 생각해보지 못했는데, 오프님 덕분에 생각의 범위를 넓히게 되었어요.
앞으로 더 많이 고민해보겠습니다. 오프님 말씀이 정말 많은 도움이 되었어요. 진심으로 감사드립니다 🙇‍♀️🙇‍♀️🙇‍♀️

Comment on lines +21 to +31
private getLinkList(): Link[] {
const commonLinks = [{ text: '홈', link: '/' }];
const authLinks = isLoggedIn()
? [
{ text: '프로필', link: '/profile' },
{ text: '로그아웃', link: '#', id: LOGOUT_ID },
]
: [{ text: '로그인', link: '/login' }];

return [...commonLinks, ...authLinks];
}
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
private getLinkList(): Link[] {
const commonLinks = [{ text: '홈', link: '/' }];
const authLinks = isLoggedIn()
? [
{ text: '프로필', link: '/profile' },
{ text: '로그아웃', link: '#', id: LOGOUT_ID },
]
: [{ text: '로그인', link: '/login' }];
return [...commonLinks, ...authLinks];
}
private getLinkList(): Link[] {
const commonLinks = [{ text: '홈', link: '/' }];
if(isLoggedIn()) {
return [...commonLinks, { text: '프로필', link: '/profile' }, { text: '로그아웃', link: '#', id: LOGOUT_ID }];
}
return [...commonLinks, { text: '로그인', link: '/login' }]
}

취향차이겠지만, 제 취향에는 요게 좀 더 맞스빈다 ㅎㅎ

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.

6 participants