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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
3 changes: 3 additions & 0 deletions src/components/posts/Post.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ export const Post = ({
time,
content,
likeUsers,
id,
activationLike = false,
onToggleLike,
}) => {
return (
<div className="bg-white rounded-lg shadow p-4 mb-4">
Expand All @@ -20,6 +22,7 @@ export const Post = ({
<p>{content}</p>
<div className="mt-2 flex justify-between text-gray-500">
<span
onClick={onToggleLike.bind(null, id)}
className={`like-button cursor-pointer${activationLike ? " text-blue-500" : ""}`}
>
Comment on lines 24 to 27

Choose a reason for hiding this comment

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

오 이렇게 함수에 파라미터를 바인딩하는 방식을 사용한 이유가 궁금해요! 클로저 함수로 생성하지 않고 globalAction에 있는 onToggleLike를 바인딩해서 함수의 재생성이나 변경을 방지하는 용도인가요 ..??
(맞다면 실제 의도한 바대로 잘 동작했는지도 궁금합니다!!)

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.

넵! 사실 props에 인라인 함수를 지향하자는 컨벤션에서 비롯된 습관?입니다. 말씀해주신대로 인라인의 경우 클릭 시 마다 함수가 재생성되는 문제를 방지할 수 있고요! 다만 정말 작은 차이이기 때문에� 성능보다는 개인적인 컨벤션이라서 해당 방식을 사용했습니다.
저도 궁금 해서 gpt에서 확인해보니
성능 최적화가 필요하지 않은 상황에서는 onClick={() => onToggleLike(id)}를 사용하는 것이 일반적입니다.
라고 하네요,,

좋아요 {likeUsers.length}
Expand Down
3 changes: 2 additions & 1 deletion src/components/posts/PostForm.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/** @jsx createVNode */
import { createVNode } from "../../lib";

export const PostForm = () => {
export const PostForm = ({ onSubmit }) => {
return (
<div className="mb-4 bg-white rounded-lg shadow p-4">
<textarea
Expand All @@ -10,6 +10,7 @@ export const PostForm = () => {
className="w-full p-2 border rounded"
/>
<button
onClick={onSubmit}
id="post-submit"
className="mt-2 bg-blue-600 text-white px-4 py-2 rounded"
>
Expand Down
7 changes: 7 additions & 0 deletions src/errors/InvalidVNodeTypeError.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export class InvalidVNodeTypeError extends Error {
static MESSAGE = "InvalidVNodeTypeError";

constructor() {
super(InvalidVNodeTypeError.MESSAGE);
}
}
Comment on lines +1 to +7

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.

저도 배우고 갑니당..!👍

Choose a reason for hiding this comment

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

배우고 갑니다.!!

49 changes: 47 additions & 2 deletions src/lib/createElement.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,50 @@
import { InvalidVNodeTypeError } from "../errors/InvalidVNodeTypeError";
import { isRenderableVNode, isTextNode } from "../utils/domUtils";
import { getEventTypeFromProps } from "../utils/eventUtils";
import { addEvent } from "./eventManager";

export function createElement(vNode) {}
export function createElement(vNode) {
if (typeof vNode === "function") {
throw new InvalidVNodeTypeError();
}
Comment on lines +7 to +9

Choose a reason for hiding this comment

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

다들 예외처리부분에서 디테일하게 잘 처리하시는 것 같습니다 bb!


function updateAttributes($el, props) {}
if (!isRenderableVNode(vNode)) {
return document.createTextNode("");
}

if (isTextNode(vNode)) {
return document.createTextNode(vNode);
}

if (Array.isArray(vNode)) {
const docFragment = document.createDocumentFragment();
const childNodes = vNode.map(createElement);
childNodes.forEach((childNode) => docFragment.appendChild(childNode));
return docFragment;
}

const $element = document.createElement(vNode.type);
for (let [key, value] of Object.entries(vNode.props || {})) {
addAttirbutes($element, key, value);
}
const childNodes = (vNode.children || []).map(createElement);
childNodes.forEach((childNode) => $element.appendChild(childNode));
return $element;
}

export const addAttirbutes = (element, key, value) => {
yeong30 marked this conversation as resolved.
Show resolved Hide resolved
if (key.startsWith("on")) {
addEvent(element, getEventTypeFromProps(key), value);
return;
}
if (key === "className") {
element.setAttribute("class", value);
return;
}
if (key === "children") {
return;
}
element.setAttribute(key, value);

return element;
};
14 changes: 12 additions & 2 deletions src/lib/createVNode.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
export function createVNode(type, props, ...children) {
return {};
import { isRenderableVNode } from "../utils/domUtils";

export function createVNode(type, props, ...childrens) {
const flatChildren = childrens
.flat(Infinity)
.filter((children) => isRenderableVNode(children));

return {
Comment on lines +3 to +7

Choose a reason for hiding this comment

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

props와 childrens에 대해 타입 체크가 있었으면 좋겠습니다 bb

type,
props,
children: flatChildren,
};
}
32 changes: 29 additions & 3 deletions src/lib/eventManager.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,31 @@
export function setupEventListeners(root) {}
const eventManager = () => {
const events = {};

export function addEvent(element, eventType, handler) {}
const scopedEventHandler = (e) => {
const targetgetEvent = events[e.type].get(e.target);
if (targetgetEvent) targetgetEvent(e);
};
Comment on lines +5 to +6

Choose a reason for hiding this comment

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

targetgetEvent라는 변수명보단 targetHandler, targetEvent 혹은 eventHandler와 같은 변수명을 고려해봐도 좋을 것 같아요!

Choose a reason for hiding this comment

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

저도 도운님과 동일한 생각인데요~!
변수/함수명을 지을때 동사+명사 조합으로 지어주시는게 좋습니다 :)
도운님의 예시를 참고해주시면 좋을 것 같아요!


export function removeEvent(element, eventType, handler) {}
const setupEventListeners = (root) => {
for (const eventType in events) {
root.removeEventListener(eventType, scopedEventHandler);
root.addEventListener(eventType, scopedEventHandler);
}
};

const addEvent = (element, eventType, handler) => {
if (!events[eventType]) {
events[eventType] = new WeakMap();
}
events[eventType].set(element, handler);
};

const removeEvent = (element, eventType, handler) => {
if (events[eventType].get(element) === handler) {
events[eventType].delete(element);
}
};

return { setupEventListeners, addEvent, removeEvent };
};
export const { setupEventListeners, addEvent, removeEvent } = eventManager();
28 changes: 27 additions & 1 deletion src/lib/normalizeVNode.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,29 @@
import { isRenderableVNode, isTextNode } from "../utils/domUtils";

export function normalizeVNode(vNode) {
return vNode;
if (!isRenderableVNode(vNode)) {
return "";
}

if (isTextNode(vNode)) {
return String(vNode);
}

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

if (Array.isArray(vNode)) {
const vNodoes = vNode.map(normalizeVNode);
return vNodoes;

Choose a reason for hiding this comment

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

오타인 것 같은데 vNodes로 수정하면 좋을 것 같습니다~

Suggested change
const vNodoes = vNode.map(normalizeVNode);
return vNodoes;
const vNodes = vNode.map(normalizeVNode);
return vNodes;

Copy link
Author

Choose a reason for hiding this comment

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

오 감사합니다! 급하게 하느라 오타가 많이 심각하네요..

}

const childNodes = vNode.children.map(normalizeVNode);
return {
...vNode,
children: childNodes,
};
}
13 changes: 10 additions & 3 deletions src/lib/renderElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,14 @@ import { normalizeVNode } from "./normalizeVNode";
import { updateElement } from "./updateElement";

export function renderElement(vNode, container) {
// 최초 렌더링시에는 createElement로 DOM을 생성하고
// 이후에는 updateElement로 기존 DOM을 업데이트한다.
// 렌더링이 완료되면 container에 이벤트를 등록한다.
const normalizedNode = normalizeVNode(vNode);

if (container.oldVNode) {
updateElement(container, normalizedNode, container.oldVNode);
} else {
const newNode = createElement(normalizedNode);
container.appendChild(newNode);
}
setupEventListeners(container);
container.oldVNode = normalizedNode;
}
61 changes: 57 additions & 4 deletions src/lib/updateElement.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,59 @@
import { addEvent, removeEvent } from "./eventManager";
import { createElement } from "./createElement.js";
import { isTextNode } from "../utils/domUtils.js";
import { getEventTypeFromProps } from "../utils/eventUtils.js";
import { addAttirbutes, createElement } from "./createElement.js";
import { removeEvent } from "./eventManager.js";

function updateAttributes(target, originNewProps, originOldProps) {}
function updateAttributes(element, newProps, oldProps) {
for (let [key, value] of Object.entries(oldProps)) {
if (key in newProps) continue;

export function updateElement(parentElement, newNode, oldNode, index = 0) {}
element.removeAttribute(key);

if (key.startsWith("on")) {
removeEvent(element, getEventTypeFromProps(key), value);
}
}
for (let [key, value] of Object.entries(newProps)) {
addAttirbutes(element, key, value);
}
}

export function updateElement(parentElement, newNode, oldNode, index = 0) {
const oldElement = parentElement.childNodes[index];

//oldNode만 있는 경우
if (oldNode && !newNode) {
parentElement.removeChild(oldElement);
return;
}

//newNode만 있는 경우
if (newNode && !oldNode) {
const newElement = createElement(newNode);
parentElement.appendChild(newElement);
return;
}
//oldNode와 newNode 모두 text 타입일 경우
if (isTextNode(newNode) && isTextNode(oldNode) && oldNode !== newNode) {
const newTextElement = document.createTextNode(newNode);
parentElement.replaceChild(newTextElement, oldElement);
return;
}
//oldNode와 newNode의 태그 이름(type)이 다를 경우
if (newNode.type !== oldNode.type) {
parentElement.replaceChild(createElement(newNode), oldElement);
return;
}

if (newNode.type === oldNode.type) {
updateAttributes(oldElement, newNode.props || {}, oldNode.props || {});
}

const oldChildren = oldNode.children || [];
Comment on lines +48 to +51

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.

이부분은 사실 위에서 반대케이스가 체크되긴 한데, 제가 어떤 케이스들이 걸리는지 알고싶어서 남겨두었었어요! 생각해보니 주석으로 남기는게 더 좋겠네요! 감사합니다

const newChildren = newNode.children || [];
const maxChildrenLength = Math.max(oldChildren.length, newChildren.length);

for (let i = 0; i < maxChildrenLength; i++) {
updateElement(oldElement, newChildren[i], oldChildren[i], i);
}
}
21 changes: 18 additions & 3 deletions src/pages/HomePage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,15 @@ import { globalStore } from "../stores";
* - 로그인하지 않은 사용자가 게시물에 좋아요를 누를 경우, "로그인 후 이용해주세요"를 alert로 띄운다.
*/
export const HomePage = () => {
const { posts } = globalStore.getState();
const { posts, loggedIn, currentUser } = globalStore.getState();
const { toggleLike, addPost } = globalStore.actions;

const handleSubmitPostForm = (e) => {
e.preventDefault();

const content = document.querySelector("#post-content").value;

Choose a reason for hiding this comment

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

querySelector로 ID를 하드코딩하여 접근하는 방식보다 event 객체(e)의 target을 이용하여 content에 접근하는 방식을 고려해보면 어떨까요?? 개인적으로는 더 안정적이고 코드 변동 시(ex. id 값의 변동) 대응이 더 쉬울 것 같은데 우영님의 의견도 궁금합니다 !!

Copy link
Author

Choose a reason for hiding this comment

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

그렇게 접근하는 방법도 있었군요.. 감사합니다!

addPost(content);
};

return (
<div className="bg-gray-100 min-h-screen flex justify-center">
Expand All @@ -20,12 +28,19 @@ export const HomePage = () => {
<Navigation />

<main className="p-4">
<PostForm />
{loggedIn && <PostForm onSubmit={handleSubmitPostForm} />}

Choose a reason for hiding this comment

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

혹시 form에서만 사용하는 이벤트라면 PostForm 컴포넌트에 선언할 수도 있었을 것 같은데 props로 전달하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

보통 하위 컴포넌트를 분리할 때, 컴포넌트 재사용성을 높이기 위해 비지니스 로직은 외부 핸들러 방식을 선호해서 다음과 같이 사용했습니다! 현재는 단순 add만 해주지만, 페이지에 따라 추가적인 처리를 해야하는 경우도 있으니까요.
다만, 원정님 피드백을 받고 생각해보니 해당 프로젝트는 PostForm이 재사용성이 없으니 내부 핸들러 방식이 더 적합한거 같습니다! 외부 핸들러를 사용하려면 아예 Form 컴포넌트로 분리 후 사용하는게 더 적합하겠어요ㅎㅎ 감사합니다!

<div id="posts-container" className="space-y-4">
{[...posts]
.sort((a, b) => b.time - a.time)
.map((props) => {
return <Post {...props} activationLike={false} />;
const isLiked = props.likeUsers.includes(currentUser?.username);
return (
<Post
{...props}
activationLike={isLiked}
onToggleLike={toggleLike}
/>
);
})}
</div>
</main>
Expand Down
32 changes: 32 additions & 0 deletions src/stores/globalStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,37 @@ export const globalStore = createStore(
userStorage.reset();
return { ...state, currentUser: null, loggedIn: false };
},
toggleLike(state, id) {
const { posts, loggedIn, currentUser } = state;

if (!loggedIn) {
alert("로그인 후 이용해주세요");
return;
}

const { username } = currentUser;

const newPosts = posts.map((post) => {
if (post.id !== id) return post;

const likeUsers = post.likeUsers.includes(username)
? post.likeUsers.filter((user) => user !== username)
: [...post.likeUsers, username];

return { ...post, likeUsers };
});
return { ...state, posts: newPosts };
},
addPost(state, content) {
const { currentUser, posts } = state;
const newPost = {
id: posts.length > 0 ? posts[posts.length - 1].id + 1 : 1,
author: currentUser.username,
time: Date.now(),
content,
likeUsers: [],
};
return { ...state, posts: [...posts, newPost] };
},
},
);
3 changes: 3 additions & 0 deletions src/utils/domUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const isTextNode = (node) => ["string", "number"].includes(typeof node);
export const isRenderableVNode = (vNode) =>
vNode != undefined && vNode != null && typeof vNode !== "boolean";
4 changes: 4 additions & 0 deletions src/utils/eventUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,7 @@ export const addEvent = (eventType, selector, handler) => {
}
eventHandlers[eventType][selector] = handler;
};

export const getEventTypeFromProps = (eventProps) => {
return eventProps.toLowerCase().slice(2, eventProps.length);
};
8 changes: 8 additions & 0 deletions vite.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,13 @@ export default mergeConfig(
setupFiles: "./src/setupTests.js",
exclude: ["**/e2e/**", "**/*.e2e.spec.js", "**/node_modules/**"],
},
resolve: {
alias: [
{
find: "@",
replacement: "/src",
},
],
},
Comment on lines +23 to +30

Choose a reason for hiding this comment

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

오 이렇게 선언하면 파일 위치가 바뀌거나 복잡한 상대경로를 좀 더 깔끔하게 선언할 수 있겠네요!! 디테일한 부분 참고하겠습니다
그런데 @로 별칭을 지정했는데 사용된 부분이 없어 이 부분도 적용하면 좋을 것 같습니다

// 기존 코드
import { getEventTypeFromProps } from "../utils/eventUtils"; 

// 수정 코드
import { getEventTypeFromProps } from "@/utils/eventUtils";

}),
);
Loading