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

[8팀 최준만] [Chapter 1-2] 프레임워크 없이 SPA 만들기 #2

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

Conversation

junman95
Copy link

@junman95 junman95 commented Dec 21, 2024

과제 체크포인트

기본과제

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

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

이벤트 위임

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

심화 과제

1) Diff 알고리즘 구현

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

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

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

과제 셀프회고

이번 주 과제는 tc가 잘 나뉘어져 있어서 스텝 바이 스텝으로 접근이 쉬웠습니다.
다만, 재귀적 동작 수행과, 속성 할당에 있어서 예외 케이스에 대한 충분한 고찰이 참 많이 필요했다고 느낍니다.
엣지 케이스를 사전에 파악하는 방법이 뭐가 있을지 생각해 보았고, 더 치밀한 tc로 해결할 수 있지 않을까 생각합니다.
이부분을 남은 시간동안 구현해보려 합니다.

기술적 성장

  • react의 기본 적인 렌더링 원리와 비교알고리즘이 어떻게 구현되어 있는지 따라 구현해 볼 수 있었습니다.
  • virtual DOM -> real DOM 변환 과정에서 디버깅이 어려운 부분들이 있었습니다.

    심화 tc가 정상적으로 통과함에도 1-1 과제 tc가 문제가 되는 경우들이었습니다.
    이과정에서 attr update 코드에서 이슈가 있음을 파악하는데 오랜 시간이 걸렸고 원인은 tc를 너무 맹신했기 때문이라 생각합니다.
    tc를 통과한 스테이지의 코드는 문제가 없을 것이라 가정하고 디버깅을 하여 잘못된 곳을 오랜시간 관찰하고 고쳐보면서 많은 시간을 썼습니다.
    결과적으로 속성 업데이트 관련 코드의 몇가지 조건에서 발생한 이슈였고 이부분을 더 꼼꼼한 tc 삽입을 통해 해결해야함을 느꼈습니다.
    tc와 커버리지의 중요성을 배웠습니다.

학습 효과 분석

  • 가상돔의 비교 과정과 렌더링 과정을 학습 하면서, 경량화 된 v-dom 간의 비교를 통해 무거운 dom과 비교를 피하는 과정을 구현해 보았습니다.
  • root에 모든 이벤트리스너를 추가하고 위임을 통해서 동작 시키는 것에 대해 1주차에서 사용한 이벤트 위임에 대한 실습을 다시 해볼 수 있었습니다.

과제 회고

구현과정

코드의 흐름

  • 초기 렌더링 시 VNode를 생성하고, 이를 실제 DOM으로 변환하여 렌더링한다.
  • 이후 상태가 변경되면 변경된 부분만 다시 렌더링한다.
    • diff 알고리즘을 통해 변경된 부분을 찾아내고, 이를 실제 DOM에 반영한다.
    • diff는 oldVNode와 newVNode를 비교하여 변경된 부분을 찾아내는 알고리즘이다.
  • 전역 이벤트는 root 엘리먼트에 이벤트를 등록하여 위임을 통해 처리한다.

1. VNode(Virtual DOM) 구현

  • VNode는 가상 DOM을 의미하며, 실제 DOM을 추상화한 객체이다.

1.1 createVNode

  1. 가상 돔을 생성하기 위해 jsx로 작성된 코드를 esbuild에서 parsing하여 트리를 만들어주어 전달한다.
  2. 해당 트리를 createVNode 함수를 통해 VNode로 변환한다.
// recursiveFlatten : 재귀적으로 배열을 펼치는 함수
export function createVNode(type, props, ...children) {
  const flattenedChildren = recursiveFlatten(children, (val) => {
    return checkNullishExceptZero(val);
  });

  return { type, props, children: flattenedChildren };
}

1.2 normalizeVNode

  • VNode를 정규화하는 함수
    • 각종 타입에 따라 VNode를 생성한다.
    • 함수형태의 컴포넌트의 경우 함수를 실행하여 VNode를 생성한다.
    • children의 경우에도 재귀적으로 normalizeVNode를 호출하여 VNode를 생성한다.
// falsy 값은 빈 문자열로 변환
// 숫자, 문자 = > 문자열로 변환
// 함수 => 함수 실행 결과로 변환

// vNode : {type , props, children}
export function normalizeVNode(vNode) {
  if (typeof vNode === "boolean" || !checkNullishExceptZero(vNode)) {
    return "";
  }

  if (typeof vNode === "number" || typeof vNode === "string") {
    return String(vNode);
  }

  if (typeof vNode === "object") {
    if (typeof vNode.type === "function") {
      return normalizeVNode(
        vNode.type({ ...vNode.props, children: vNode.children }),
      );
    }

    return {
      type: vNode.type,
      props: vNode.props,
      children: vNode.children
        .map(normalizeVNode)
        .filter(checkNullishExceptZero),
    };
  }
}

1.3 createElement

  • VNode를 실제 DOM으로 변환하는 함수
    • VNode의 타입에 따라 다른 처리를 한다.
    • VNode의 children을 재귀적으로 createElement를 호출하여 변환한다.
export function createElement(vNode) {
  if (typeof vNode === "function" || typeof vNode?.type === "function") {
    throw new Error("NEED NORMALIZE");
  }
  if (typeof vNode === "boolean" || !checkNullishExceptZero(vNode)) {
    return document.createTextNode("");
  }

  // 문자열 또는 숫자인 경우 텍스트 노드로 처리
  if (typeof vNode === "string" || typeof vNode === "number") {
    return document.createTextNode(String(vNode));
  }

  // 배열로 들어온 경우 fragment로 처리
  if (Array.isArray(vNode)) {
    const frag = document.createDocumentFragment();
    vNode.forEach((node) => {
      frag.append(makeElement(node));
    });
    return frag;
  }

  // 그 외의 경우 element로 처리
  const el = makeElement(vNode);

  if (Array.isArray(vNode.children)) {
    vNode.children.forEach((child) => {
      el.appendChild(createElement(child));
    });
  }

  return el;
}

function makeElement(vNode) {
  const el = document.createElement(vNode.type);
  updateAttributes(el, vNode.props);
  return el;
}
  • 속성 추가시에는 이벤트 속성, class속성, 그 외의 속성을 구분해야 한다.
function updateAttributes($el, props) {
  for (const prop in props) {
    if (prop?.startsWith("on")) {
      // 이벤트 속성
      addEvent($el, prop.slice(2).toLowerCase(), props[prop]);
      continue;
    }
    if (prop === "class") {
      // class 속성
      $el.classList.add(props[prop]);
      continue;
    }
    $el.setAttribute(prop, props[prop]); // class 속성과 나머지 속성 동시 처리
  }
}

1.4 render

  • 생성된 DOM element를 container element에 간단히 append 하여 렌더링한다.
export function renderElement(vNode, container) {
  //생략
  container.appendChild(createElement(vNode));
}

1.5 setupEventListeners

  • 이벤트 위임을 통해 전역 이벤트를 처리한다.
    • root 엘리먼트에 이벤트를 등록하여 위임을 통해 처리한다.

먼저, setup을 해주기 이전에, createElement에서 이벤트 속성을 넣어주는 부분을 살펴보자(1.5.1~1.5.2)

1.5.1 updateAttributes

  • createElement 함수에서 이벤트 속성을 처리할 때, 이벤트 위임을 통해 전역 이벤트를 처리한다.
// createElement 내부에서 이벤트를 전역 이벤트 관리 객체에 넣어주는 코드는 attr할당 부분에 있음
function updateAttributes($el, props) {
  for (const prop in props) {
    if (prop?.startsWith("on")) {
      addEvent($el, prop.slice(2).toLowerCase(), props[prop]); // 이벤트 전역 이벤트 관리 객체에 넣어주는 코드
      continue;
    }
    $el.setAttribute(replaceIfPropIsClass(prop), props[prop]);
  }
}

1.5.2 addEvent

  • 위 코드에서 이벤트를 전역객체에 넣어주는 동작은 아래 addEvent 함수에서 처리한다.
    • 이벤트 타입에 따라 전역 이벤트 객체에 이벤트를 등록한다.
    • 이벤트 타입별 전역 이벤트 객체를 WeakMap으로 관리한다.
      • WeakMap은 key가 null이 되면 GC에 의해 메모리에서 해제된다.
      • WeakMap의 특성을 이용해, key를 DOM element로, value를 이벤트 핸들러로 관리한다.
        • 이벤트 핸들러는 DOM element에 대한 참조를 가지고 있으므로, DOM element가 GC에 의해 해제되면 이벤트 핸들러도 해제된다.
globalEvents의 형태
{
  eventType1 : WeakMap {
    element1 : handler1,
    element2 : handler2,
    ...
  },
  eventType2 : WeakMap {
    element1 : handler1,
    element2 : handler2,
    ...
  },
  ...
}
export function addEvent(element, eventType, handler) {
  if (!element || typeof handler !== "function") return;

  globalEvents[eventType] = globalEvents[eventType] || new WeakMap();
  globalEvents[eventType].set(element, handler);
}

export function removeEvent(element, eventType, handler) {
  if (globalEvents[eventType].get(element) === handler) {
    globalEvents[eventType].delete(element);
  }
}

1.5.3 setupEventListeners

  • 모두 렌더링된 후에, setupEventListeners를 통해 전역 이벤트를 등록한다.
export function setupEventListeners(root) {
  for (const eventType in globalEvents) {
    root.removeEventListener(eventType, handleGlobalEvents);
    root.addEventListener(eventType, handleGlobalEvents);
  }
}

export function handleGlobalEvents(e) {
  if (globalEvents[e.type].has(e.target)) {
    globalEvents[e.type].get(e.target)(e);
  }
}

종결(?)

  • 여기까지가 최초 렌더링을 위한 VNode 생성과 실제 DOM 변환, 이벤트 위임까지의 과정이다.

  • 이후에는 상태 변경에 따른 렌더링을 위한 diff 알고리즘을 구현하고, 이를 통해 변경된 부분만 다시 렌더링하는 과정을 서술한다.

    • diff가 없이 전부 repaint하는 방식을 짤막하게 보여주며 마무리한다.

      클릭해서 열기
      export function renderElement(vNode, container) {
        // step0. 이벤트 초기화
        clearEvents();
      
        // step1. vNode 정규화
        if (!vNode) {
          return;
        }
        const normalizedVNode = normalizeVNode(vNode);
      
        // step2. 정규화된 vNode를 기반으로 DOM 생성 또는 업데이트
        const element = createElement(normalizedVNode);
      
        // step3. DOM 초기화 및 업데이트
        container.innerHTML = ""; // container 초기화
        container.append(element); // container에 DOM 추가
      
        // step4. 이벤트 핸들러 등록
        setupEventListeners(container);
      }

2. diff 알고리즘 구현

  • dom 업데이트를 위해 이전 vNode와 새로운 vNode를 비교하여 변경된 부분만 업데이트한다.
  • 1.1~1.2 까지는 동일한 과정을 수행하고 이전 vNode가 존재하는 경우에만 diff 알고리즘을 수행한다.

2.0 최초 렌더링 후 oldVNode 저장

  • 최초 렌더링 후 oldVNode를 저장한다.
let oldVNode = null;
export function render(vNode, container) {
  // 생략 - 1.1~1.5
  oldVNode = vNode; // 최초 렌더링 후 oldVNode 저장
}

2.1 updateElement

  • diff 알고리즘을 통해 변경된 부분만 업데이트한다.

2.1.1 oldVNode 존재 여부에 따른 분기 처리

  • oldVNode가 존재하는 경우에만 diff 알고리즘을 수행한다.
export function renderElement(vNode, container) {
  if (!vNode) {
    return;
  }
  // step1. vNode 정규화
  const newVNode = normalizeVNode(vNode);

  // step2. 정규화된 vNode를 기반으로 DOM 생성 또는 업데이트
  if (!oldVNode) {
    // step3. DOM 추가
    const element = createElement(newVNode);
    container.append(element); // 첫 렌더링 때만 container에 DOM 추가
  } else {
    // step3-diff. DOM 업데이트
    if (container.firstChild) {
      updateElement(container, newVNode, oldVNode);
    } else {
      const element = createElement(newVNode);
      container.append(element);
    }
  }
  //생략
}

2.1.2 updateElement

  • diff 알고리즘을 통해 변경된 부분만 업데이트한다.
    • newNode와 oldNode를 비교하여 변경된 부분만 업데이트한다.
      • 기존 노드가 없는 경우 : appendChild
      • 업데이트 될 노드가 없는 경우 : removeChild
      • 타입이 다른 경우 : replaceChild
      • 텍스트 노드인 경우 : textContent 업데이트
      • 타입이 같은 경우 : props 및 children 비교
export function updateElement(parentElement, newNode, oldNode, index = 0) {
  if (newNode && !oldNode) {
    parentElement.appendChild(createElement(newNode));
    return;
  }

  // newNode가 없는 경우 oldNode 제거
  if (oldNode && !newNode) {
    parentElement.removeChild(parentElement.childNodes[index]);
    return;
  }

  // 텍스트 노드인 경우
  if (typeof newNode === "string" || typeof newNode === "number") {
    if (oldNode !== newNode) {
      parentElement.childNodes[index].textContent = newNode;
    }
    return;
  }

  // newNode가 oldNode와 다른 타입인 경우
  if (newNode.type !== oldNode.type) {
    parentElement.replaceChild(
      createElement(newNode),
      parentElement.childNodes[index],
    );
    return;
  }

  // 타입이 같은 경우 props 및 children 비교
  updateAttributes(
    parentElement.childNodes[index],
    newNode.props || {},
    oldNode.props || {},
  );

  // children이 없는 경우 return
  if (Math.max(newNode.children.length, oldNode.children.length) === 0) {
    return;
  }

  const maxLength = Math.max(newNode.children.length, oldNode.children.length);
  for (let i = 0; i < maxLength; i++) {
    updateElement(
      parentElement.childNodes[index],
      newNode.children[i],
      oldNode.children[i],
      i,
    );
  }
}
  • maxLength : children 비교 시, 모든 자식요소를 비교하기 위해 maxLength를 구하고, 이를 기반으로 반복문을 수행한다.

2.1.3 updateAttributes

  • props를 비교하여 변경된 부분만 업데이트한다.
    • oldProps를 기준으로 newProps를 비교하여 변경된 부분만 업데이트한다.
    • 신규로 추가된 props는 oldProps 업데이트 이후 추가한다.
function updateAttributes(target, originNewProps, originOldProps) {
  const newPropsArr = originNewProps ? Object.entries(originNewProps) : [];
  const oldPropsArr = originOldProps ? Object.entries(originOldProps) : [];

  for (const [prop, value] of oldPropsArr) {
    let newPropValue = originNewProps[prop];

    // 이벤트 프로퍼티인 경우 이벤트 제거하고 시작
    if (isEventProp(prop)) {
      removeEvent(target, replaceEventProp(prop), value);
      newPropValue = originNewProps[replaceEventProp(prop)];
    }

    // 새로 들어올 프로퍼티 값이 없는 경우 제거
    if (newPropValue === undefined) {
      target.removeAttribute(prop);
      continue;
    }

    // 새로 들어올 프로퍼티 값이 있는 경우 업데이트
    if (value !== newPropValue) {
      if (isClass(prop)) {
        target.classList = newPropValue;
        continue;
      }
      if (isEventProp(prop)) {
        addEvent(target, replaceEventProp(prop), newPropValue);
        continue;
      }

      target.setAttribute(prop, newPropValue);
      continue;
    }

    // 새로 들어올 프로퍼티 값이 같은 경우
    if (value === originNewProps[prop]) {
      continue;
    }
  }

  for (const [prop, value] of newPropsArr) {
    // 이전 프로퍼티에 없는 프로퍼티인 경우 추가
    if (oldPropsArr[prop] === undefined) {
      if (isEventProp(prop)) {
        addEvent(target, replaceEventProp(prop), value);
        return;
      }
      if (isClass(prop)) {
        target.classList = value;
        continue;
      }
      target.setAttribute(prop, value);
    }
  }
}

2.2 render 와 eventListener

  • 1의 과정과 동일하게 renderElement 함수를 통해 렌더링과 이벤트 위임 동작을 수행한다.

2.3 종결!

  • 여기까지가 diff 알고리즘을 통해 변경된 부분만 업데이트하는 과정이다.

##마치며
가상돔을 이용해 렌더링 과정을 구현하면서 하나의 문장으로 정리하자면,
** 비교는 가상돔끼리, 표현은 실제 돔에! ** 라고 할 수 있겠다.

가상돔끼리의 비교를 통해 실제 돔과 비교보다 훨씬 가볍게 변경된 부분을 포착하고 업데이트할 수 있었다.
선언적으로 저장된 함수를 실행시켜 가상돔을 만들고, 이를 실제 돔으로 변환하여 렌더링하는 과정을 통해 리액트의 동작 원리에 대한 이해를 높일 수 있었다.

리뷰 받고 싶은 내용

  • 개선 혹은 추가적으로 구현해 보면 좋을 만한 내용이 있으면 키워드 주시면 좋을 것 같습니다~!

@junman95 junman95 changed the title WIP [8팀 최준만]Chapter 1-2. 프레임워크 없이 SPA 만들기 Part 2 WIP [8팀 최준만] [Chapter 1-2] 프레임워크 없이 SPA 만들기 Dec 21, 2024
* 재귀적 평탄화 유틸 함수 구현
* 헬퍼 함수로 0을 제외 한 값은 falsy 체크를 통한 삭제
* 0을 제외한 falsy값 찾는 함수 유틸화
* 정규화 종료 단계에서 props 합쳐주기 로직 구현(이부분 조언 필요)
* input vNode가 배열일 경우, 재귀 실행
* 스트링, falsy등 처리 끝나면 dom 요소 생성
* createElement시 on~ 으로 시작하는 prop의 경우 addEvent처리
* WeakMap 자료형을 사용하여 등록될 이벤트 리스트 생성
* 업데이트일 수 도 있으니, 이벤트 전부 제거하고 시작
* container를 비우고 새로 append하도록 구현
* todo : 속성 교체 로직 구현
* MuataionObserver를 사용해 element 옵저빙 및 childList 제거 동작 포착 시 oldVNode 초기화
src/stores/globalStore.js Outdated Show resolved Hide resolved
src/lib/updateElement.js Outdated Show resolved Hide resolved
Comment on lines 1 to 18
export function createElementChildRemoveObserver(elements, helper) {
const observer = new MutationObserver((mutations) => {
mutations.forEach((mutation) => {
if (mutation.type === "childList") {
mutation.removedNodes.forEach((node) => {
console.log(node, "removed");
helper(node);
});
}
});
});

for (const element of elements) {
observer.observe(element, { childList: true, subtree: false });
}

return observer;
}

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.

디버깅하면서 없앴는데 다시한번 붙여봐야겠네요 ㅋㅋ

Copy link
Author

Choose a reason for hiding this comment

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

제가 다시 붙여보려 했는데, 사실 상위 엘리먼트에서 요소를 지운다 하더라도 oldVNode를 무조건 초기화 하는 것은 좋지 않을 것 같아요.
가령 layout이 같은 두개의 다른 페이지(프로필, 홈)를 오갈때, updateElement가 아닌 createElement를 시키는 것이 더 비효율적일 테니까요!
MutationObserver 자체는 충분히 사용 방안에 대해서 공부해볼만한 좋은 WebAPI 인 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

덕분에 최적한 방안에 대해서 생각해 볼 수 있어 좋았습니다. 감사해요!

src/lib/renderElement.js Show resolved Hide resolved
Copy link

@devJayve devJayve 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 +3 to +7
export function checkNullishExceptZero(value) {
// 0은 falsy한 값이 아니고 숫자로 처리할 것이다.
if (value === 0) return true;
return Boolean(value);
}

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.

오 0일때는 true를 리턴하는 구문이 앞에 있으니까 `value === null || typeof value === "undefined" ..."로 0을 제외한 falsey한 값을 일일히 작성할 필요가 없겠네요!👍

src/__tests__/chapter1-2/custom.test.jsx Show resolved Hide resolved
Comment on lines +18 to +20
time: Date.now(),
content: document.getElementById("post-content").value,
likeUsers: [],

Choose a reason for hiding this comment

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

document.getElementById로 ID를 하드코딩하는 것보다 event를 prop으로 받아서 event.target으로 접근하는 방법이 더 안정적일 것 같은데 어떻게 생각히사는지요?!

Copy link
Author

Choose a reason for hiding this comment

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

어떤식으로 해야할지 살짝 감이 오지 않습니다.
참고할 커밋 있을까요?
내일 같이 코드리뷰 시간에 얘기해보면 좋을 것 같아요!

globalStore.setState({
posts: [
{
id: posts[0].id + 1,

Choose a reason for hiding this comment

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

지금은 posts가 기본 더미 데이터로 설정되어있긴한데 posts가 비어있다면? 이라는 생각이 들었어요 (저도 사실 고민하다가 Date.now()로 넣어놓고 따로 수정은 안했네요.. ㅎㅎ 더 좋은 방법 있는지 고민해봐도 좋을 것 같습니다)

Copy link
Author

Choose a reason for hiding this comment

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

id: posts ? posts.length + 1 : 1,

요래 바꿔봤는데 어떤가요? @devJayve

Copy link
Author

Choose a reason for hiding this comment

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

저는 store쪽 보시면 id값을 내림차순으로 변경해 놨습니다.
가장 최근에 들어온게 가장 아이디가 크도록요!

저희 과제에선 삭제가 없어서 이렇게 처리했습니다 ㅋㅋ

Copy link
Author

Choose a reason for hiding this comment

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

20ff127

tc와 함께 구현해봤습니다.

src/lib/normalizeVNode.js Outdated Show resolved Hide resolved
globalEvents = {};
}

window.__myEventListeners = globalEvents;

Choose a reason for hiding this comment

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

현재 이벤트 처리가 전역을 대상으로 이루어지긴하는데 root 컨테이너 하위에서만 이벤트가 발생하니 setupEventListeners 내부에서 root 컨테이너에 대하여 설정해주는 건 어떠신가요 ??

export function setupEventListeners(root) {
    root.__myEventListeners = globalEvents; // 예시
}

Copy link
Author

Choose a reason for hiding this comment

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

앗 해먼드 코치님이 브라우저 콘솔에서 window.~ 으로 바로 확인해서 디버깅 하시는 것 보고 유용하다 싶어 썼고 삭제 해야할 코드입니다.
전역으로 접근 가능하게 하면 사실 매우매우 위험할 것 같네요.
확인 감사드려요~!

*/
export function setupEventListeners(root) {
for (const eventType in globalEvents) {
root.removeEventListener(eventType, handleGlobalEvents);

Choose a reason for hiding this comment

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

혹시 updateAttributes에서도 removeEvent를 해주는데 root에서 한 번 더 삭제해주는 이유가 있을까요??

Copy link
Author

Choose a reason for hiding this comment

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

와 진짜 좋은 피드백인것 같습니다.

제 설계의도는 속성 업데이트 시 명령적으로 제거하는 것이 맞습니다.
위 코드는 이전에 근백님과 함께 디버깅 하면서 근백님의 코드 스타일을 제가 고려 없이 그대로 반영하다가 들어간 코드입니다.

예리한 피드백 감사합니다!

addEvent($el, prop.slice(2).toLowerCase(), props[prop]);
continue;
}
$el.setAttribute(replaceIfPropIsClass(prop), props[prop]);

Choose a reason for hiding this comment

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

개인적으로는 replacePropIsClass로 별도의 함수로 뺐다면, 윗줄에서 한 번 실행하고 반환값을 받아서 prop을 인자로 넘겨주는 쪽이 가독성이 더 좋을 것 같아요!

src/__tests__/chapter1-2/custom.test.jsx Show resolved Hide resolved
Comment on lines +3 to +7
export function checkNullishExceptZero(value) {
// 0은 falsy한 값이 아니고 숫자로 처리할 것이다.
if (value === 0) return true;
return Boolean(value);
}

Choose a reason for hiding this comment

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

오 0일때는 true를 리턴하는 구문이 앞에 있으니까 `value === null || typeof value === "undefined" ..."로 0을 제외한 falsey한 값을 일일히 작성할 필요가 없겠네요!👍

Copy link

@osohyun0224 osohyun0224 left a comment

Choose a reason for hiding this comment

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

안녕하세요 준만님 ~! 담당 학습메이트 오소현입니다 ㅎㅎ

준만님 요번주에 PR 구체적으로 작성해주시고, 구현한 함수들에 대한 개념도 굉장히 잘 작성해주셔서 제가 코드를 이해하면서 정말 많은 도움이 되었습니다 :) 신경써서 PR 작성해주셔서 너무 감사드려요~! 앞으로도 기대하겠습니다 ㅎㅎ

그리고 준만님께서 따로 구현한 함수에 대해 따로 테스트코드를 추가하신것 엄청 좋네요 ㅎㅎ 뭔가 테스트 코드 처음 짜보시기 어렵지 않으셨나요? 함수 제대로 불러와서 테스트 초기화부터 기댓값까지 엄청 잘 작성해주신 것 같습니다 :) 정말 너무 잘해주셨어요 ><

요번주에는 도운님 근백님께서 굉장히 리뷰를 잘 남겨주셔서 저도 팀원분들의 리뷰를보면서 같이 고민한 점도 있고, 제가 직접 실행하면서 더 제안해보면 좋을 점을 살포시 작성해보았습니다 ^0^ 부족하지만 제 리뷰가 준만님께서 이번주 과제를 다시한번 더 되돌아 볼 수 있는 좋은 기회가 되었으면 좋겠습니다 🫶🏻🫶🏻

크리스마스에도 all day,,,,과제를 열심히 해주셔서 너무 감사했어요 :) 쉬는 날없이 달리는 준만님,,! 최고!!
이번주 과제도 너무 고생 많으셨고, 다음주 과제도 화이팅입니다 아자자~!

Comment on lines +3 to +10
------------------24.12.25 02:14---------------------

basic 테스트 코드 통과
챕터 1-1 테스트 코드 통과

------------------24.12.26 03:00---------------------

advanced 테스트 코드 통과

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.

타임라인처럼 보고 해결에 며칠 걸렸는지 보기도 좋을것 같아서 그랬는데, 큰 의미는 없습니다 ㅎㅎ

README.md Show resolved Hide resolved
src/lib/createElement.js Outdated Show resolved Hide resolved
Comment on lines 12 to 16
const onClickAddPost = () => {
globalStore.setState({
posts: [
{
id: posts[0].id + 1,

Choose a reason for hiding this comment

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

굿 저도 비슷한 생각을 가지고 있는데요!
저는 그래서 위의 코드에서 posts가 비어있을 때 에러가 발생할 수 있을때를 고려한 새 게시물의 id를 추가하는 id를 다음과 같이 추가로 구현해보면 어떨까 싶어요! 제가 지금 코드를 보고 생각한 코드는 다음과 같은데, 추가로 더 고려해보시면 좋을 것 같습니다!

Suggested change
const onClickAddPost = () => {
globalStore.setState({
posts: [
{
id: posts[0].id + 1,
const onClickAddPost = () => {
const newId = posts.length > 0 ? Math.max(...posts.map(post => post.id)) + 1 : 1;
globalStore.setState({
posts: [
{
id: newId,

제가 보고 구현해본 내용은 다음과 같은데 테스트 코드는 통과하지만 개인적으로 마음에는 안드는 구조예요 ㅎㅎ,, 준만님께서 더 고민해보시면 좋을 것 같습니다:)

Copy link
Author

Choose a reason for hiding this comment

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

정말 비슷한 코드를 원정님 커밋에서 봤는데, 제가 거기에 대안으로 코드를 작성해봤어요. 한번 보시고 이것도 리뷰해 주시죵 ㅎㅎ

id: posts.reduce((acc,cur)=> {
       if(acc < cur) return cur;
       return acc;
     },0) + 1;

Copy link
Author

Choose a reason for hiding this comment

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

[] 빈배열이 들어가도 1이 나오고, 가장 큰 값에 +1을 더하는 것은 동일합니다.

Comment on lines +53 to +61
function updateAttributes($el, props) {
for (const prop in props) {
if (prop?.startsWith("on")) {
addEvent($el, prop.slice(2).toLowerCase(), props[prop]);
continue;
}
$el.setAttribute(replaceIfPropIsClass(prop), props[prop]);
}
}

Choose a reason for hiding this comment

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

updateAttributes 함수를 보면 기존 코드는 모든 속성을 매번 설정하고 있어서 DOM 요소에 이미 설정된 속성이라도 변경 없이도 불필요하게 다시 설정되고 있네요 혹시 맞나용??

Suggested change
function updateAttributes($el, props) {
for (const prop in props) {
if (prop?.startsWith("on")) {
addEvent($el, prop.slice(2).toLowerCase(), props[prop]);
continue;
}
$el.setAttribute(replaceIfPropIsClass(prop), props[prop]);
}
}
function updateAttributes($el, props, prevProps = {}) {
for (const key in prevProps) {
if (!(key in props)) {
$el.removeAttribute(key);
}
}
for (const key in props) {
if (props[key] !== prevProps[key]) {
if (key.startsWith('on')) {
addEvent($el, key.slice(2).toLowerCase(), props[key]);
} else if (key === 'className') {
$el.setAttribute('class', props[key]);
} else {
$el.setAttribute(key, props[key]);
}
}
}
}

제가 개선해보면서 addEvent 함수를 호출하기 전에 이벤트 리스너 속성이 변경되었는지 확인하는 로직까지 한번 추가 해봤는데 요렇게 추가로 해보시는건 어떤가요?? 조금 더 고도화 하면 좋을 것 같습니다 :) 이 함수의 역할이 잘 보이게 간결하게 잘 작성하신 것 같아요!! 좋습니다 👍

Copy link
Author

Choose a reason for hiding this comment

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

오 코드 개선까지 감사합니다.

여기서 궁금한게,
props[key] !== prevProps[key] 코드에서 핸들러 함수가 상이한 것을 체크할 수 있나요??

Comment on lines 1 to 11
export function createElementChildRemoveObserver(elements, helper) {
const observer = new MutationObserver((mutations) => {
mutations.forEach((mutation) => {
if (mutation.type === "childList") {
mutation.removedNodes.forEach((node) => {
console.log(node, "removed");
helper(node);
});
}
});
});

Choose a reason for hiding this comment

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

이번 과제에서는 따로 사용되지 않아보이는데 코드를 읽어보면 도입하시려는 목적이 DOM 요소들의 자식 요소가 제거되고 나서, 각 제거된 요소에 대해 따로 이벤트 처리를 하시려고 한 것 같네요 어떤 목적으로 구현하려고 하셨나요??? 궁금합니다👀👀

Copy link
Author

Choose a reason for hiding this comment

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

아~ MutationObserver를 통해서 최상위 엘리먼트 root 혹은 body등에서 삭제되는 순간 oldVNode를 초기화 해 주기 위해 넣은 코드입니다. 하지만 코드 개발 과정에서 초기화 과정이 불필요하다고 판단하여(오히려 성능 저하시킴)
뺴게 되었습니다!!

초기에 넣게된 이유는 테스트코드에서 before,afterEach등의 동작에서 자꾸 dom을 초기화 하는데 이부분 때문에 이슈가 난다고 판단해서 해당 타이밍을 포착해 다 초기화 해주려고 한 것이었습니다.

Comment on lines 17 to 22
export function setupEventListeners(root) {
for (const eventType in globalEvents) {
root.removeEventListener(eventType, handleGlobalEvents);
root.addEventListener(eventType, handleGlobalEvents);
}
}

Choose a reason for hiding this comment

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

제가 @chb6734 분의 코드를 함께 보다가 setupEventListeners 함수에 필요할 때만 이벤트 리스너를 등록하도록 처음 등록할때만 이벤트를 추가하도록 하는것도 좋을 것 같아요~!
현재 코드에서는 이벤트 리스너를 제거한 다음 다시 추가하는 로직을 구현하고 있는데 처음 등록할때에만 추가하는 것도 좋을 것 같습니다

Suggested change
export function setupEventListeners(root) {
for (const eventType in globalEvents) {
root.removeEventListener(eventType, handleGlobalEvents);
root.addEventListener(eventType, handleGlobalEvents);
}
}
export function setupEventListeners(root) {
for (const eventType in globalEvents) {
if (!root.getAttribute(`data-listener-${eventType}`)) {
root.addEventListener(eventType, handleGlobalEvents);
root.setAttribute(`data-listener-${eventType}`, 'true');
}
}
}

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 +27 to +31
/**
* @description 이벤트 프로퍼티인 경우 on 제거하고 소문자로 변경
* @param {string} prop
* @returns {string}
*/

Choose a reason for hiding this comment

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

js doc 잘 활용해 주셔서 가독성이 올라갔어요~!~! 너무 잘하셨습니다 ㅎㅎ 저는 주로 메인 로직 함수에만 잘 작성해주는 편인데, 준만님의 주석을 작성해주시는 기준이 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

감삼당

저는 네이밍 만으로는 설명이안되거나,
vscode에서 js의 경우 jsdoc에 작성된 type에 맞춰 자동완성을 할 수 있게 보조해 주는 기능이 있어서 씁니다.

HTMLElement의 내부 속성과 메서드가 참 많잖아요?
자동완성 없이 쓰면 어떤게 있는지 아주 자주쓰는 것을 제외하고는 알수가 없더라구요!

Copy link
Author

@junman95 junman95 Dec 27, 2024

Choose a reason for hiding this comment

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

  1. jsdoc 안썼을때!
    image
    image
    속성 못찾음.

  2. jsdoc 에 parameter type 명시해 줬을 때!
    image
    image
    속성 훤히보인당

Comment on lines +42 to +44
export function clearEvents() {
globalEvents = {};
}

Choose a reason for hiding this comment

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

이 함수의 목적이 전역 globalEvents 객체를 완전히 초기화 하는 역할을 하고 있는 것 같아요!

이렇게 globalEvents = {}; 구현해주셔도 좋은데 개별 이벤트 유형에 대해 이벤트 리스너를 제거하도록 각각 세부적으로 구현해보는 것도 좋은 것 같아요 :)

Suggested change
export function clearEvents() {
globalEvents = {};
}
export function clearEvents() {
for (const eventType in globalEvents) {
const eventMap = globalEvents[eventType];
eventMap.forEach((_, element) => {
if (element && element.removeEventListener) {
element.removeEventListener(eventType, handleGlobalEvents);
}
});
delete globalEvents[eventType];
}
}

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 +7 to +17
export function recursiveFlatten(arr, helper = () => true) {
return arr.reduce((acc, cur) => {
if (Array.isArray(cur)) {
const flattenedChild = recursiveFlatten(cur, helper);
acc = acc.concat(flattenedChild);
} else {
if (helper(cur)) acc.push(cur);
}
return acc;
}, []);
}

Choose a reason for hiding this comment

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

배열 병합하실때 concat 메소드를 사용하시는 군요,,

https://codingeverybody.kr/%EC%9E%90%EB%B0%94%EC%8A%A4%ED%81%AC%EB%A6%BD%ED%8A%B8-concat-%ED%95%A8%EC%88%98/

concat 이랑 push를 둘다 사용하셨는데 저는 둘다 push로 한번 구현해봤습니다 ㅎㅎ

Suggested change
export function recursiveFlatten(arr, helper = () => true) {
return arr.reduce((acc, cur) => {
if (Array.isArray(cur)) {
const flattenedChild = recursiveFlatten(cur, helper);
acc = acc.concat(flattenedChild);
} else {
if (helper(cur)) acc.push(cur);
}
return acc;
}, []);
}
export function recursiveFlatten(arr, helper = () => true) {
return arr.reduce((acc, cur) => {
if (Array.isArray(cur)) {
const flattenedChild = recursiveFlatten(cur, helper);
acc.push(...flattenedChild); // spread 연산자를 사용하여 배열 병합을 최적화
} else if (helper(cur)) {
acc.push(cur);
}
return acc;
}, []);
}

concat ()을 자주 사용하시나용??

Copy link
Author

Choose a reason for hiding this comment

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

push를 쓰게되면 스프레드 해주어야하고 이과정에서. 불필요한 메모리가 낭비되지 않을까~ 라고 생각했습니당!

Copy link

@yeong30 yeong30 left a comment

Choose a reason for hiding this comment

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

eventManager.js를 참고하려 왔다가 준만님의 노력과 고민이 가득하신 코드를 보고 많이 반성했습니다🥲 정말 많이 고민하신게 느껴져요!
특히 jsdoc이나 테스트 코드등 과제에서 놓치기 쉬운부분까지 신경쓰신거 대단하십니다👍👍
제가 보면서 궁금한점 몇개 코멘트로 달아두었습니다 많이 배우고 갑니다!

}
}

export function handleGlobalEvents(e) {
Copy link

Choose a reason for hiding this comment

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

eventManager에서 중복된 이벤트 이슈를 어떻게 해결해야할지 감이 안잡혔는데, 준만님 코드를 보고 많이 배웠습니다!👍👍👍

src/__tests__/chapter1-2/custom.test.jsx Show resolved Hide resolved
@@ -20,7 +22,8 @@ export const Post = ({
<p>{content}</p>
<div className="mt-2 flex justify-between text-gray-500">
<span
className={`like-button cursor-pointer${activationLike ? " text-blue-500" : ""}`}
className={`like-button cursor-pointer${likeUsers.includes(currentUser?.username) ? " text-blue-500" : ""}`}
Copy link

Choose a reason for hiding this comment

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

저는 제시된 activationLike를 그대로 사용했는데, 어차피 likeUsers를 전달해주니, 굳이 위에서 체크할 필요가 없었군요..
꼼꼼하게 체크하신게 느껴집니다 👍

import { addEvent } from "./eventManager";

export function createElement(vNode) {}
export function createElement(vNode) {
if (typeof vNode === "function" || typeof vNode?.type === "function") {
Copy link

Choose a reason for hiding this comment

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

normalizeVNode.jsvNode.type === "function"를 통해
컴포넌트 함수는 다 처리된 후 createElement로 전달될 것 같은데 혹시 한번 더 체크하시는 이유가 있으신지, 예외케이스가 있으셨는지 궁금합니다!

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 +21 to +27
if (Array.isArray(vNode)) {
const frag = document.createDocumentFragment();
vNode.forEach((node) => {
frag.append(makeElement(node));
});
return frag;
}
Copy link

Choose a reason for hiding this comment

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

여기서 vNode가 배열일 경우 createElement가 아니라 makeElement로 처리해주셨는데, 배열 요소에 vNode === "string" || vNode === "boolean"요런 케이스에 해당하는 요소가 올 경우에 오류가 발생하지 않으셨는지 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

진짜 문제의 소지가 있겠네요!!
리뷰 감사합니다. 고쳐야될 코드같아요!!

@junman95 junman95 changed the title WIP [8팀 최준만] [Chapter 1-2] 프레임워크 없이 SPA 만들기 [8팀 최준만] [Chapter 1-2] 프레임워크 없이 SPA 만들기 Dec 28, 2024
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