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

[2팀 노정우] [Chapter 1-2] 프레임워크 없이 SPA 만들기 Part 2 #6

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

melroh629
Copy link

@melroh629 melroh629 commented Dec 23, 2024

과제 체크포인트

기본과제

가상돔을 기반으로 렌더링하기

  • createVNode 함수를 이용하여 vNode를 만든다.
  • normalizeVNode 함수를 이용하여 vNode를 정규화한다.
  • createElement 함수를 이용하여 vNode를 실제 DOM으로 만든다.
  • 결과적으로, JSX를 실제 DOM으로 변환할 수 있도록 만들었다.

이벤트 위임

  • 노드를 생성할 때 이벤트를 직접 등록하는게 아니라 이벤트 위임 방식으로 등록해야 한다
  • 동적으로 추가된 요소에도 이벤트가 정상적으로 작동해야 한다
  • 이벤트 핸들러가 제거되면 더 이상 호출되지 않아야 한다

심화과제

1) Diff 알고리즘 구현

  • 초기 렌더링이 올바르게 수행되어야 한다
  • diff 알고리즘을 통해 변경된 부분만 업데이트해야 한다
  • 새로운 요소를 추가하고 불필요한 요소를 제거해야 한다
  • 요소의 속성만 변경되었을 때 요소를 재사용해야 한다
  • 요소의 타입이 변경되었을 때 새로운 요소를 생성해야 한다

2) 포스트 추가/좋아요 기능 구현

  • 비사용자는 포스트 작성 폼이 보이지 않는다
  • 비사용자는 포스트에 좋아요를 클릭할 경우, 경고 메세지가 발생한다.
  • 사용자는 포스트 작성 폼이 보인다.
  • 사용자는 포스트를 추가할 수 있다.
  • 사용자는 포스트에 좋아요를 클릭할 경우, 좋아요가 토글된다.

과제 셀프회고

기술적 성장

  • weakMap
  • Map, Set을 처음으로 써본 것 같다(?)
  • createDocumentFragment 활용

코드 품질

  • 함수로 만들 수 있는 것들이 있을 것 같은데 조건문으로만 처리한 부분이 많아 아쉽다. 분명 개선 시킬 수 있는 방법이 있을 것 같은데 아직은 생각이 나지 않는다.

학습 효과 분석

  • 이전에도 JS로 리액트 렌더링에 도전한 경험이 있는데, 그때는 정말 맨땅에 헤딩으로 했던 도전이라 결국 실패만 남았었던 기억이.. 이번에는 코치님과 AI 덕분에 완성 시킬 수 있었지만 리액트의 렌더링(diff 알고리즘, fiber개념)에 대해서 조금 더 공부를 하고 정리해봐도 좋을 것 같다.

과제 피드백

  • 과제를 어떻게 시작해야 할 지 감이 오지 않아서, AI의 도움을 받아 완성했습니다. 이후 새롭게 브랜치를 파서 AI 도움으로 완성했던 코드를 다시 따라했습니다. 이렇게 하니까 어떻게 과제를 해야 할지 감도 잡히고 진척도 있었지만 이게 맞는지는 모르겠습니다.
  • 이벤트 버블링을 언제 사용하면 좋을지 잘 모르겠어요. 이벤트 버블링 활용해 보세요 이런 언급이 있다면 사용하겠지만 그렇지 않으면 이벤트 버블링을 스스로 생각해내지는 못할 것 같습니다.

리뷰 받고 싶은 내용

  1. updateAttribute 타입에러
function updateAttributes(target, newProps = {}, oldProps = {}) {
  // null이나 undefined인 경우 빈 객체로 처리
  oldProps = oldProps || {};

  newProps = newProps || {};
  ...
}
  • normalizeVNode 함수에서 이미 null이나 undefined를 빈 문자열로 변환하고, 객체가 아닌 경우도 처리
  • updateElement 함수를 보면, props를 전달하기 전에 이미 노드의 유효성 검사
  • 따라서 updateAttributes에 전달되는 newProps와 oldProps는 이미 정제된 값이어야 함

위와 같은 이유로 내부에 있던 null & undefined 체크 코드를 지웠는데 타입에러가 납니다. 왜 그런걸까요?

  1. createVNode

1️⃣ type, props, children을 각각 따로 리턴

export function createVNode(type, props, ...children) {
    ...
   return { type, props, children: flatChildren };
}

2️⃣ props에 children을 포함시켜서 리턴

export function createVNode(type, props, ...children) {
  ...
  return {
    type,
    props: {
      ...props,
      children: flatChildren,
    },
  };
}

types, props, children을 각각 따로 관리하는 게 좋을것 같아서 개별적으로 return 해줬습니다.
이후에 리팩토링 단계에서 생각해보니 normalizeVNode에서 children배열 처리를 너무 복잡하게 하고 있는 것 같아, 애초에 children을 props에 포함시켜도 좋을 것 같다는 생각을 했습니다. 어떤게 더 나은 방법일까요?

// 6. children 배열처리
  if (Array.isArray(vNode.children)) {
    const normalizedChildren = vNode.children // 여기서 children에 한번 더 접근을 해야한다는 부분이 낭비라고 생각했습니다.
      .map((child) => normalizeVNode(child))
      .filter((child) => child !== "" && child != null && child !== false);

    return {
      ...vNode,
      children: normalizedChildren,
    };
  }
  return vNode

@melroh629 melroh629 changed the title [2팀 노정우] [2팀 노정우] [Chapter 1-2] 프레임워크 없이 SPA 만들기 Part 2 Dec 23, 2024
@melroh629 melroh629 marked this pull request as ready for review December 26, 2024 01:52
@gmkim716
Copy link

AI에 도움을 받았다고 하셨지만 코드 작성 과정에 고민을 많이 하셨던 흔적이 보인 것 같습니다! (저도 리팩토링에 신경을 써야 하는데 말이죠)

type, props, children에서 props와 children의 역할이 비슷하면서도 조금 다른 걸로 이해하고 있어요. 속성과 자식 노드를 포함하는 것에 차이가 있는 것처럼이요(틀릴 수 있어서 조심..) 코드를 단순화할 수 있는 측면에서는 포함시키는 게 좋다고 저도 생각하고 있습니다. 혹시 멘토님 리뷰가 있었나요? 같이 고민해보면 좋을 것 같습니다

Comment on lines +8 to +11
const { loggedIn, posts, currentUser } = globalStore.getState();
const activationLike =
currentUser && likeUsers.includes(currentUser.username);

Choose a reason for hiding this comment

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

activationLike를 Post 내부에서 선언해주셨네요 👍
관심사 분리의 목적이었을까요? 의견이 궁금합니다 !

Comment on lines +52 to +53
} else if (key === "htmlFor") {
$el.setAttribute("for", value);

Choose a reason for hiding this comment

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

디테일이네요.. htmlFor일 경우 for로 대체해주어야 하는군요 !

궁금해서 블로그도 찾아봤네요 :) 👍
https://despiteallthat.tistory.com/179

Choose a reason for hiding this comment

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

오.. 감사합니다

elementHandlers.forEach((handler) => {
handler.call(target, event);
});
if (!event.bubbles) break;

Choose a reason for hiding this comment

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

오.. bubbles 프로퍼티를 사용하면 이렇게 버블링하지 않을 때 처리를 할 수 있군요!...👍

Choose a reason for hiding this comment

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

저도 몰랐습니다ㅎㅎ 감사합니다

Copy link

@2dowon 2dowon left a comment

Choose a reason for hiding this comment

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

제가 이번주 과제를 제대로 못했더니, 내용적으로 리뷰를 못했습니다..!
다음에 과제 복습하고 다시 리뷰하러 올게요! 그래도 전체적으로 주석이 잘 달려 있어 잘 읽혔어요!

  • if문 사용하실 때 inline하게 쓰시는 곳도 있고, bracket 사용하시는 곳도 있는 것 같아요~ 하나로 통일되면 좋을 것 같습니다!
  • TestPage가 있는데 혹시 어떤 어떻게 사용하는 페이지인가요? 과제 범위는 아니었던 것 같아서 의도가 있을 것 같아 실행시켜 봤는데 어떻게 사용하는 페이지인지 잘 모르겠어서 궁금해서 여쭤봅니당 ㅎㅎ

Copy link

@hdlee0619 hdlee0619 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 +52 to +53
} else if (key === "htmlFor") {
$el.setAttribute("for", value);

Choose a reason for hiding this comment

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

오.. 감사합니다

elementHandlers.forEach((handler) => {
handler.call(target, event);
});
if (!event.bubbles) break;

Choose a reason for hiding this comment

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

저도 몰랐습니다ㅎㅎ 감사합니다

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