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 만들기 #45

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

Conversation

Songchangyeop
Copy link

@Songchangyeop Songchangyeop commented Dec 26, 2024

과제 체크포인트

기본과제

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

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

이벤트 위임

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

심화 과제

1) Diff 알고리즘 구현

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

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

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

과제 셀프회고

이번주는 개인적인 일정이 너무 많이 잡혀 있어서 과제에 온전히 집중하지 못 했던 한 주 였습니다..
점심시간이나, 새벽 시간을 활용해서 과제를 해보려 노력은 했지만 처음 과제를 보고 이해를 하기까지의 과정이 너무 오래걸렸던 것 같아요
벽에 막힌 듯한 기분을 마구마구 느끼며 과제를 진행했습니다
UI와 관련된 기능을 개발하는 부분은 익숙해서 저번주 과제는 어렵지만 머릿속에서 그림을 그려내면서 개발을 했던 것 같은데 이번주 과제는 제가 평소 쓰던 뇌와 다른 뇌를 쓰는 기분이라 정말 어려웠던 것 같습니다

잘한점

  • 과제를 시작하기전 학습 자료를 꼼꼼히 읽어보면서 이해하려고 노력한 것 (결국 완벽하게 이해는 못 했습니다 ㅠ)
  • 시간을 쪼개고 쪼개가며 테스트 통과를 위해 시간을 투자한 것

아쉬운점

  • 몇달전부터 잡힌 일정 때문에 과제에 온전히 집중하지 못한 것
  • 저번주보다 더 GPT에 의존한 것..
  • 목요일 밤에 업보를 청산하느라 리팩토링에 신경을 못쓴 것

기술적 성장

이번 과제를 수행하면서 어떻게 jsx로 작성하는 코드가 어떤신으로 변환되어 페이지에 노출이 될 수 있게 되는지 알게되는 유익한 시간이었습니다
또 어떻게 DOM에서 변경되는 부분만 리렌더링을 발생시키는지 완벽하진 않지만 알게 되었습니다 그 과정을 이해하기까지 너무 오래걸려서 GPT와 함께 같이 이해하려고 노력했습니다..
과제와는 별개로 익숙하지 않은 타입스크립트를 자바스크립트 코드에 도입하면서 타입을 잘 처리하는 능력이 점점 늘고 있는 것 같습니다..

코드 품질

정말 부끄럽지만 이번 과제는 먼저 올린 분들의 PR + AI + 이게 되네? 가 만들어준 코드인 것 같습니다
코드의 품질을 나름 신경쓰려고 했지만 시간이 부족하다는 핑계로 리팩토링 시간을 투자하지 못 해서 우선 돌아가게 만들자!가 이번주 저의 큰 목표였던 것 같습니다 품질 부분이 제일 아쉽습니다.

학습 효과 분석

처음엔 어디서부터 어떻게 손을 대야할지 몰라서 천장을 보며 한숨을 많이 쉬었던 것 같습니다
그렇게 혼자 고민하다 다른분들 PR을 보거나 학습자료를 찾아보며 최대한 이해하려고 노력을 많이 했었습니다
결과적으로 완벽하게는 아니지만 리액트에서 jsx를 어떻게 처리하는지, 부분적으로 변경되는 부분만 찾아 렌더링을 어떻게 하는지 조금이나마 이해를 한 것 같습니다
시간이 많이 부족해서 완벽하게 이해하지 못해 아쉽지만 시간을 내서 꼭 이해하는 시간을 가지고 기록을 해 봐야 겠다고 생각했습니다

과제 피드백

평소에 리액트를 개발하며 어떻게 동작하는지 개념적으로만 알고있던 부분을 실제로 구현해보면서 느낀 게 많았습니다
이번주엔 시간 투자를 저번주의 절반도 못 해서 개인적으로 너무 아쉬웠던 주 였던 것 같습니다
이번주 과제에서 얻어 갈 수 있는게 너무 많은 것 같아서 꼭 시간을 내서 더 공부하는 시간을 가지려고 합니다

리뷰 받고 싶은 내용

코드를 작성하면서 2가지 방향을 고민하고 있는데 어떤 게 더 나은 구조인지 고민이 됩니다.

updateElement 함수를 호출할 때 속성을 변경해주는 작업을 해주게 되는데 이 때 새로운 prop�s와 이전의 props을 비교해 추가,삭제, 수정을 해주게 됩니다

아래는 기존의 코드인데요

이전 속성을 제거하고 새로운 속성을 추가하는 과정 자체를 하나의 for 루프에서 처리를 해주고 있었습니다
이렇게 초기에 작성한 이유는 하나의 루프에서 처리를 해주기 때문에 반복 횟수를 줄일 수 있다고 생각했어요

function updateAttributes(
  target: HTMLElement,
  newProps: Record<string, any>,
  oldProps: Record<string, any>,
) {
  const allKeys = new Set([...Object.keys(oldProps), ...Object.keys(newProps)]);

  for (const key of allKeys) {
    const oldValue = oldProps[key];
    const newValue = newProps[key];

    if (!(key in newProps)) {
      if (key.startsWith("on")) {
        const eventName = key.toLowerCase().substring(2);
        removeEvent(target, eventName);
      } else {
        target.removeAttribute(key === "className" ? "class" : key);
      }
    } else if (oldValue !== newValue) {
      if (key.startsWith("on")) {
        const eventName = key.toLowerCase().substring(2);
        if (oldValue) removeEvent(target, eventName);
        if (newValue) addEvent(target, eventName, newValue);
      } else {
        target.setAttribute(key === "className" ? "class" : key, newValue);
      }
    }
  }
}

그런데 코드를 작성하면서 문득 든 생각이 �추가, 삭제, 수정 로직이 하나의 블록에 묶이면서 가독성 저하 가능성이 있지 않을까 라는 생각이 들었고, 역할을 분리해주어야겠다는 생각이 들어 결국 아래 구조로 변경하게 되었어요

결국 두개의 루프가 돌게 되었지만 좀 더 역할이 명확해져서 코드의 파악이 쉬워지지 않았나 하는 생각이 듭니다
또 반면에 이 코드는 렌더링 속도에 영향을 끼치는 코드내용이기 때문에 이런 구조를 가져가도 되나 하는 고민이 들었습니다

// 이전 속성 제거
  for (const key in oldProps) {
    if (!(key in newProps)) {
      if (key.startsWith("on")) {
        const eventName = key.toLowerCase().substring(2);
        removeEvent(target, eventName);
      } else {
        target.removeAttribute(key === "className" ? "class" : key);
      }
    }
  }

  // 새로운 속성 설정 또는 업데이트
  for (const key in newProps) {
    const oldValue = oldProps[key];
    const newValue = newProps[key];

    if (oldValue !== newValue) {
      if (key.startsWith("on")) {
        const eventName = key.toLowerCase().substring(2);
        if (oldValue) {
          removeEvent(target, eventName);
        }
        if (newValue) {
          addEvent(target, eventName, newValue);
        }
      } else {
        if (key === "className") {
          target.setAttribute("class", newValue);
        } else {
          target.setAttribute(key, newValue);
        }
      }
    }
  }

코치님이 판단하시기에 어떤 방향이 더 나아 보이는지 의견이 궁금합니다 !


추가적으로 리뷰와 별개로 저번주, 이번주 과제를 하면서 느낀점인데 제가 혼자 개발을 하다보니 어느지점에서 끊고 커밋을 해야하는지 헷갈리는 것 같습니다 지금은 기능 구현, 수정 처럼 큰 기준을 잡고 커밋을 했는데 어떤 지점에서 커밋을 해야 협업하는 사람이 더 편하게 이해할 수 있는지 궁금합니다 !

Copy link

@lshyun955 lshyun955 left a comment

Choose a reason for hiding this comment

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

많이 배웠습니다 창엽님!!ㅎㅎㅎ
코드가 정말 깔끔해서 읽기편하네요.....!

updateAttributes함수를 저도 창엽님 코드를 참고해서 작성했는데,,,ㅎ
PR 코멘트에 남기신 부분처럼 프로젝트가 커졌을 상황을 고려하면 아무래도 함수 자체가 렌더링 시 호출되는 함수다보니..
루프를 1개로 줄이는게 성능상 좋지 않을까?? 라는 의견을 남겨봅니다 ㅎ...ㅎ

@gmkim716
Copy link

창엽님 PR을 보면서 코드 리뷰어를 위해 어떻게 PR을 작성해야 하는지 배워가는 것 같습니다. 블로그를 보듯이 흐름 파악이 쉬워서 어떤 고민을 하셨는지가 잘 읽히는 것 같�아요

이번주 창엽님께서 많이 바쁘셨단걸 알기에 막히는 지점에 대해 많이 고통스러우셨을 것 같은 주간이었습니다. 조금은 고민을 내려놓고 안되는 부분에 있어서 언제든 대화를 나눠보시면 더 효율적으로 공부하실 수 있을 것 같아요. (이해도 중요하지만, 어떻게 써먹어야 할지를 더 고민해봐야한다는 개인적인 의견입니다) 빠르게 과제 마무리하고 딮한 고민을 같이 나누면 좋을 것 같습니다

2개의 루프를 도는 코드로의 변경이 효율적일지에 대해서 기술적인 피드백을 드리진 못하지만(ㅜ) 이해하기 편한 코드와 성능을 고려한 코드에 대해 저도 같이 고민해갈 수 있도록 하겠습니다. (과제를 빨리 끝내고 이런 고민들을 나누면 좋을것 같아요)

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.

한 주도 고생하셨습니다ㅎㅎ

@@ -44,6 +44,7 @@
"jsdom": "^25.0.1",
"lint-staged": "^15.2.11",
"prettier": "^3.4.2",
"typescript": "^5.7.2",

Choose a reason for hiding this comment

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

타입스크립트 사용하셨군요.. 저도 다음 과제에는 꼭 타입스크립트로 한번 해보겠습니다..

Choose a reason for hiding this comment

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

22..

Copy link

Choose a reason for hiding this comment

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

333

Copy link
Author

Choose a reason for hiding this comment

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

이번 과제부터 타입스크립트가 기본 적용이니 같이 타입지옥에 빠져보시지요 ㅎㅎㅎ

Comment on lines +5 to +10
interface LinkProps {
onClick?: () => void;
children?: unknown;
href: string;
className?: string;
}

Choose a reason for hiding this comment

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

저는 타입스크립트를 많이 안 써봤습니다..ㅎㅎ
그래서 궁금한게 이렇게 보통 props를 넘겨줄 때, interface 아니면 type도 쓰는 것 같던데 보통 뭘 더 많이 쓰시나요??

Choose a reason for hiding this comment

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

https://yceffort.kr/2021/03/typescript-interface-vs-type
저도 이게 궁금해서 찾아보다가 도움이 되었던 블로그를 공유드려요! 🤓

Copy link
Author

Choose a reason for hiding this comment

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

수연님 좋은 아티클 감사드립니다 ㅎㅎ

이건 별건 아니구 타입스크립트의 문제를 풀어보는 챌린지인데 한번 보셔도 될 것 같습니다 ! (저는 eazy도 어려웠어요)
타입스크립트에 익숙해지는데 도움이 되는 것 같습니다 :)

https://github.com/type-challenges/type-challenges

@Suyeon-B
Copy link

Suyeon-B commented Dec 28, 2024

저도 창엽님과 같은 고민을 하고 결국 다음과 같이 합쳤어요. 🥹

export function updateAttributes(target, newProps, oldProps) {
  const allProps = new Set([
    ...Object.keys(oldProps || {}),
    ...Object.keys(newProps || {}),
  ]);

  allProps.forEach((attr) => {
    if (attr === "children") return;

    const oldValue = oldProps?.[attr];
    const newValue = newProps?.[attr];

    if (attr.startsWith("on")) {
      const eventType = attr.slice(2).toLowerCase();

      if (newValue !== oldValue) {
        if (oldValue) {
          removeEvent(target, eventType, oldValue);
        }
        if (newValue) {
          addEvent(target, eventType, newValue);
        }
      }
      return;
    }

    if (newValue === undefined) {
      target.removeAttribute(attr);
      return;
    }

    const attributeName = attr === "className" ? "class" : attr;
    target.setAttribute(attributeName, newValue);
  });
}

코치님께 어떤 피드백을 받으셨는지 궁금하네요!!! 🤓
이번주 동반 업보 청산 입대(?) 동지로서... 짧은 시간 내에 이렇게 멋지게 과제를 해내신 점 너무 대단하다고 생각합니다.
고생 많으셨습니다!!! 👏

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.

이번주에 시간이 많이 없으셨던 걸로 알고 있었는데, 이렇게 과제 다 끝내신거 보니 진짜 대단합니다...
👍👍 고생 많으셨습니다!

(+) 개인적으로 반복문은 줄일 수 있으면 줄이는게 맞지 않을까라고 생각해봅니다..! 그리고 하나의 반복문으로 되어 있는 부분도 주석 추가하고 그러면 가독성이 그렇게 떨어지지 않는다고 생각해요!

export function Link({ onClick, children, ...props }: LinkProps) {
const handleClick = (e) => {
e.preventDefault();
onClick?.();
Copy link

Choose a reason for hiding this comment

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

저는 보통 onClick && onClick() 이렇게 사용했었는데, onClick?.() 이렇게 쓰면 onClick이 함수인 경우에만 실행되는 것이라서 더 안전하다고 하네요!
알아갑니다!! 👍👍

Comment on lines +4 to +6
const 초 = 1000;
const 분 = 초 * 60;
const 시간 = 분 * 60;
Copy link

Choose a reason for hiding this comment

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

한글 변수명은 신선한데, 가독성이 좋네요!ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

아하 이건 원래 기존 코드내부에 있었어요 ! ㅎㅎ

별개로 저는 한글 변수는 가끔 쓰기도 합니다! 변수명이 길어지거나 위의 예시처럼 시간을 처리할 경우 가독성이 한글이 더 좋더라고요 !

@@ -44,6 +44,7 @@
"jsdom": "^25.0.1",
"lint-staged": "^15.2.11",
"prettier": "^3.4.2",
"typescript": "^5.7.2",
Copy link

Choose a reason for hiding this comment

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

333

Copy link

@wonyoung2257 wonyoung2257 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 +18 to +19
const { loggedIn } = globalStore.getState();
const { toggleLike } = globalStore.actions;

Choose a reason for hiding this comment

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

Post를 호출하는 곳에서 조회하면 더 좋을 것 같아요
loggedIn 이라는 값은 globalStore에서 한번만 조회도 되는 값이라 Post에서 하는 것 보단 Post를 호출하는 부모 컴포넌트에서 사용해서 props으로 넘겨주는게 더 좋지 않을까 싶습니다.

Copy link
Author

Choose a reason for hiding this comment

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

앗 그러네요 ! 원영님 말씀대로 생각하니 더 좋은 구조인 것 같습니다
좋은의견 감사합니다 :)

@Songchangyeop
Copy link
Author

Songchangyeop commented Dec 28, 2024

@2dowon @Suyeon-B @lshyun955 @gmkim716

코치님 피드백 공유드립니다 ㅎㅎ

스크린샷 2024-12-28 오후 4 48 16

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.

7 participants