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

[Mission5/이찬] Project_Notion_VanillaJs 과제 #47

Open
wants to merge 2 commits into
base: fe/4/1eecan
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!DOCTYPE html>
<html lang="ko">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Observer</title>
Copy link
Member

Choose a reason for hiding this comment

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

Observer pattern에 대한 고민이 느껴집니다.. 저도 시도해보려다 포기해서 아쉬움이 많습니다.. 😂

Choose a reason for hiding this comment

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

ㅋㅋ 제목이 비장하네요 ㅋㅋㅋ

</head>
<body>
<div
id="app"
style="display: flex; flex-direction: row; justify-content: space-around"
></div>
Comment on lines +9 to +12

Choose a reason for hiding this comment

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

ask;

여기에 style을 넣은 이유가 무엇인지 알 수 있을까요??

이 프로젝트에서는 큰 문제가 없긴하나.
SPA를 하다보면 초기 html 세팅 이후 html 파일을 건드리는 경우가 많지 않다보니
여기서 특정 속성을 넣게 되었을 때 원인을 발견하기 쉽지 않은 경우가 꽤 있더라구요.

main.js 부터 보다보니 나는 딱히 뭐 건든게 없는데 왜 레이아웃이 계속 틀어지지..? 같은?

그래서 저는 이런 템플릿 파일에서의 변형은 최소한으로 가져가려 해요.

<script type="module" src="/src/main.js"></script>
</body>
</html>
111 changes: 111 additions & 0 deletions src/App.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import DocumentList from "./component/DocumentList.js";
import Document from "./component/Document.js";
import { initRouter, push } from "./core/router.js";
import nestedObjectToArray from "./core/nestedObjectToArray.js";
import { getItem, setItem } from "./core/storage.js";
import { request } from "./core/api.js";
import editor from "./component/Editor.js";

export default function App({ $target }) {
this.state = getItem("tempSaveKey") || alert("새로고침을 해주세요");
Copy link
Member

Choose a reason for hiding this comment

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

초기 렌더링을 한 후 storage의 'tempSaveKey' key에 있는 value의 유무를 확인해서 재렌더링을 하면 alert("새로고침을 해주세요")를 하실 필요가 없을 것 같습니다!! 😆

Choose a reason for hiding this comment

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

P5;

소소 리빙 포인트 Nullish coalescing operator (??) 에 대해 알고 계신가요?

||의 경우 0 또는 false, '' 와 같이 boolean으로 형 변환을 한 뒤에 false이면 뒤의 값을 할당하는 구조이다보니 만약 정말 ''이나 0, false가 의도한 값일 경우 의도와 다르게 동작하는 상황이 발생해요!

그에비해 ??의 경우 null이나 undefined와 같이 정말 값이 없을 경우만 핸들링 하죠.
그렇기에 이와 비슷한경우 ??을 추천드려요!


this.setState = async (nextState) => {
this.state = nextState;
await documentList.setState(this.state);
await document.setState(this.state);
Comment on lines +13 to +15
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
this.state = nextState;
await documentList.setState(this.state);
await document.setState(this.state);
this.state = nextState;
await documentList.setState(this.state);
await document.setState(this.state);

p5: 가독성이 좋은 코드를 고민하시는 것 같아서... 저는 이런 경우 개행을 두는 편입니다!! 훨씬 읽기 편한 것 같더라구요 😆(같은 것끼리 묶기!)

Choose a reason for hiding this comment

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

단순 궁금증.

setState가 async 하게 동작하는 이유는 무엇인가요?

};

this.fetchDocument = async () => {
if (getItem("tempSaveKey") == null) {
const serverDocument = nestedObjectToArray(
await request("/documents", {
method: "GET",
})
);
Comment on lines +21 to +24

Choose a reason for hiding this comment

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

p2;

App.js에서는 App을 init 하는 과정에서

  • 어떤 값을 전달해 주는지?
  • 어떤 컴포넌트(또는 페이지)로 구성이 되어있는지?
  • 어떤 값과 어떤 컴포넌트가 어떻게 엃혀 있는지?
    정도가 여기서 알면 좋을 정보라고 생각돼요.

그렇기에 어떤 데이터를 가져온다 정도만 알면 좋을 것 같은데,
지금 이 코드의 경우 도큐먼트라는 Path에서 request method는 get으로 데이터를 요청해서 가져오고 있어. 와 같이 너무 자세한 내용을 보여주고 있다고 생각해요.

이런 fetch 코드들은 별도의 함수로 모아 관리하면 어떨까요?

이후에 path가 바뀌거나 처리하는 값들이 달라진다면 보다 용이하게 관리할 수 있을 것 같아서요!

setItem("tempSaveKey", {

Choose a reason for hiding this comment

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

P1;

현재 tempSaveKey가 여러 곳에서 쓰이는데 별도의 상수로 관리하면 좋을 것 같아요.
이후에 값이 변경되더라도 최소한의 변경으로 핸들링 할 수 있기 때문이에요.

Choose a reason for hiding this comment

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

P3;

객체지향의 Class의 getter와 setter가 있는 것 처럼 localStorage를 사용해보면 어떨까요?
보다 명시적일 수 도 있을 것 같아서요!

const initStorage = (key: string) => {
  return {
    get: () => getItem(key);
    set: (value) => setItem(key, value);
};

const tempSaveStorage = initStorage('tempSaveKey');

tempSaveStorage.get();

이런 구조는 실제로도 많이 사용되는 패턴이에요.

이를 잘 쓰기 위해서는 클로저 에 대해 공부해보시면 좋아요!

documentList: serverDocument,
currentDocumentId: serverDocument[0].id,
});
Comment on lines +20 to +28

Choose a reason for hiding this comment

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

P5;

물론 해당 코드 또한 잘 작성되었지만, request가 하는 일이 워낙 크기 때문에 변수로 한번 짚고 코드를 작성해도 좋았을 것 같아요.

Suggested change
const serverDocument = nestedObjectToArray(
await request("/documents", {
method: "GET",
})
);
setItem("tempSaveKey", {
documentList: serverDocument,
currentDocumentId: serverDocument[0].id,
});
const serverDocumentList = await request('/document', {...});
const currentDocumentList = nestedObjectToArray(serverDocumentList);
setItem("tempSaveKey", {
documentList: currentDocumentList,
currentDocumentId: currentDocumentList[0].id,
});

}
await this.setState(getItem("tempSaveKey"));
};

this.fetchDocument();

const documentList = new DocumentList({
$target,
initialState: this.state,
onClick: async (clickedId) => {
const nextState = {
documentList: nestedObjectToArray(
await documentList.fetchDocumentList()
),
currentDocumentId: clickedId,
};
setItem("tempSaveKey", nextState);
},
onPost: async (clickedId) => {
await request("/documents", {
method: "POST",
body: JSON.stringify({
title: "문서 제목",
parent: clickedId,
}),
});
const nextState = {
documentList: nestedObjectToArray(
await documentList.fetchDocumentList()
),
currentDocumentId:
clickedId == "null"
? getItem("tempSaveKey").currentDocumentId
: clickedId,
Comment on lines +60 to +62
Copy link

@nayeon-hub nayeon-hub Jul 10, 2023

Choose a reason for hiding this comment

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

p4 : 값 비교 시 동등 연산자 ( == ) 대신 일치연산자( === ) 을 쓰면 좋을 것 같습니다😊

Choose a reason for hiding this comment

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

ask;

TMI
=====는 똑같이 값을 비교하는 연산자인데 어떤 차이가 있을까요?

  • 동작 방식
  • 성능
    측면에서 어떤 차이가 있을지 쓰윽 살펴보셔도 좋을 것 같아요.

};
this.setState(nextState);
setItem("tempSaveKey", nextState);
},
onDelete: async (clickedId) => {
await request(`/documents/${clickedId}`, {
method: "DELETE",
});
const nextState = {
documentList: nestedObjectToArray(
await documentList.fetchDocumentList()
),
currentDocumentId:
getItem("tempSaveKey").documentList[
getItem("tempSaveKey").documentList.findIndex(
(document) => document.id == clickedId
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
(document) => document.id == clickedId
(document) => document.id === Number(clickedId)

==으로 하면 동일한 값이지만 다른 타입인 값이 같은 값으로 인식이 될 수 있기 때문에 위와 같이 형변환을 해주고 "엄격한" 비교를 해주시면 좋을 것 같습니다. 동치 비교 및 동일성 참고하시면 좋을 것 같아요!! 😀

) - 1
].id,
};
push(`/documents/${nextState.currentDocumentId}`);
this.setState(nextState);
setItem("tempSaveKey", nextState);
},
});

const document = new Document({
$target,
initialState: this.state,
});

this.route = async () => {
const { pathname } = window.location;
if (pathname === "/") {
await this.setState({ ...this.state, currentDocumentId: null });
} else if (pathname.indexOf("/documents/") === 0) {
const splitedPathname = pathname.split("/");
const documentId = splitedPathname[2];
Comment on lines +98 to +99

Choose a reason for hiding this comment

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

ask;

TMI
나중에 path 구조가 /document/:id 에서 /pages/some-list/:id와 같이 바뀌게 될 경우 해당 코드도 맞춰서 고쳐야 할 것 같은데요!
시간이 꽤 지나면 이런 코드가 있었는지? 바로 알아채기 쉽지 않을 것 같아요. 그래서 휴먼에러를 발생할 수 있을 것 같은데요.

이를 예방하거나 변경되었을 때 여기도 같이 챙겨야 함을 어떻게 하면 놓치지 않고 잘 알수 있을까요?

const nextState = {
...this.state,
currentDocumentId: Number(documentId),
};
await this.setState(nextState);
}
Comment on lines +95 to +105

Choose a reason for hiding this comment

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

P5;

early return 패턴을 써도 좋았을 것 같아요.

};

this.route();

initRouter(() => this.route());
}
44 changes: 44 additions & 0 deletions src/component/Document.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { request } from "../core/api.js";
import Editor from "./Editor.js";
import nestedObjectToArray from "../core/nestedObjectToArray.js";
export default function Document({ $target, initialState }) {
const $document = document.createElement("div");
$target.appendChild($document);
this.state = initialState;
this.setState = async (nextState) => {
this.state = nextState;
await editor.setState(this.state);
await this.render();
};
Comment on lines +5 to +12
Copy link
Member

Choose a reason for hiding this comment

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

줄바꿈(개행)을 해주시면 가독성 측면에서 좋을 것 같습니다!! 😁


this.render = async () => {
$target.appendChild($document);
};

this.render();

let timer = null;

const editor = new Editor({
$target: $document,
initialState: this.state,
onEditing: async (document) => {
const documentId = this.state.currentDocumentId;
if (timer !== null) {
clearTimeout(timer);
}
timer = setTimeout(async () => {
await request(`/documents/${documentId}`, {
method: "PUT",
body: JSON.stringify(document),
});
const nextState = nestedObjectToArray(
await request(`/documents/${documentId}`, {
method: "GET",
})
);
setItem("tempSaveKey", nextState);
}, 1000);
},
Comment on lines +20 to +42

Choose a reason for hiding this comment

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

P4; ask;

요 timer를 이렇게 써도 되지만 별도의 함수로 뽑아두면 두고두고 쓸 수 있을 것 같아요.
만약 재사용성을 고려한다면 어떻게 만들어 볼 수 있을까요?

});
}
82 changes: 82 additions & 0 deletions src/component/DocumentList.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import { setItem } from "../core/storage.js";
import { push } from "../core/router.js";
import { request } from "../core/api.js";
export default function DocumentList({
$target,
initialState,
onClick,
onPost,
onDelete,
}) {
const $documentList = document.createElement("div");
$target.appendChild($documentList);
this.state = initialState;

this.setState = async (nextState) => {
this.state = nextState;
await this.render();
};

this.render = async () => {
if (this.state.documentList.length == 0) return;
Copy link
Member

Choose a reason for hiding this comment

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

👍👍👍

const documentId = this.state.currentDocumentId;
$documentList.innerHTML = `<div class="documentListItem">${this.state.documentList
.map(
(document) => `
<p id="${document.id}" style="text-indent:${
document.depth * 10

Choose a reason for hiding this comment

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

depth*10은 왜 해주는 건가요? 제가 뭔가 놓친 부분이 있는지.......🐣

Copy link
Author

Choose a reason for hiding this comment

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

text-indent 속성이 들여쓰기를 해주는 속성입니다! 따라서 depth*10을 해주면 문서목록의 들여쓰기를 해줄 수가 있습니다!

}px; background-color:${
document.id == documentId ? "gray" : "white"
}">${
document.title
}<button class="postDocument">+</button><button class="deleteDocument">x</button></p>
`
)
.join("")}<p id="null"><button class="postDocument">+</button></p></div>`;
};

this.render();

this.fetchDocumentList = async () => {
let res = await request("/documents", {
method: "GET",
});

if (res.length == 0) {
await request("/documents", {
method: "POST",
body: JSON.stringify({
title: "문서 제목",
parent: null,
}),
});
res = await request("/documents", {
method: "GET",
});
}

Choose a reason for hiding this comment

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

res.length===0일 때 새로운 문서가 무조건 생성되는 걸까요?


return res;
};

$documentList.addEventListener("click", (e) => {
if (e.target.id != "") {
const { id } = e.target;
const nextState = onClick(id);
setItem("tempSaveKey", nextState);
this.setState(nextState);
push(`/documents/${id}`);
}
});

$documentList.addEventListener("click", (e) => {
const { className } = e.target;
const { id } = e.target.closest("p");

if (className == "postDocument") {
onPost(id);
}
if (className == "deleteDocument") {
onDelete(id);
}
});
Comment on lines +61 to +81
Copy link
Member

Choose a reason for hiding this comment

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

$documentList에 대한 click 이벤트 처리를 중복으로 하시는 것 같아 보입니다!
이벤트 위임 처리를 해주면 코드의 중복을 줄일 수 있을 것 같습니다! 😆 🙋🏻‍♂️

Choose a reason for hiding this comment

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

굿입니다!!

}
49 changes: 49 additions & 0 deletions src/component/Editor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { request } from "../core/api.js";

export default function Editor({
$target,
initialState = { title: "", content: "" },
onEditing,
}) {
const $editor = document.createElement("div");

let isInitialize = false;

this.state = initialState;
$editor.style.width = "300px";
$editor.style.height = "300px";
$target.appendChild($editor);

this.setState = async (nextState) => {
this.state = nextState;
const documentId = this.state.currentDocumentId;
const currentDocument = await request(`/documents/${documentId}`, {
method: "GET",
});
$editor.querySelector("[name=title]").value = currentDocument.title;
$editor.querySelector("[name=content]").value = currentDocument.content;
this.render();
};

this.render = async () => {
const documentId = this.state.currentDocumentId;
const currentDocument = await request(`/documents/${documentId}`, {
method: "GET",
});

Choose a reason for hiding this comment

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

this.setState에서 documentId에대한 api요청을 request하였는데 state의 변화가 일어나지 않았지만 this.render에서 다시 똑같은 api요청을 request한 것처럼 보여요 👀 따로 빼두시거나 한번만 요청하는 걸로 불필요한 api요청을 줄이는게 나을 듯 합니다! 😄

if (!isInitialize) {
$editor.innerHTML = `
<input type="text" name="title" style="width:300px;" value="${currentDocument.title}"/>
<textarea name="content" style="width:300px;height:300px;">${currentDocument.content}</textarea>`;
isInitialize = true;
}
};
this.render();

$editor.addEventListener("keyup", async (e) => {
const document = {
title: $editor.querySelector("[name=title]").value,
content: $editor.querySelector("[name=content]").value,
};
onEditing(document);
});
}
21 changes: 21 additions & 0 deletions src/core/api.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
export const API_END_POINT = "https://kdt-frontend.programmers.co.kr";

export const request = async (url, option = {}) => {
try {
const res = await fetch(`${API_END_POINT}${url}`, {
...option,
headers: {
"Content-Type": "application/json",
"x-username": "1eecan",
},
});

if (res.ok) {
return await res.json();
}

Comment on lines +13 to +16

Choose a reason for hiding this comment

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

P3;

res.ok가 아닌 경우에는 어떻게 핸들링 하나요?
4xx번대 에러와 5xx번대 에러의 경우 각기 다르게 핸들링 해야할 것 같은데 여기서는 throw new Error("API 처리 중 이상 발생"); 로 퉁쳐지는 것 같아서요!

Choose a reason for hiding this comment

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

ask; 퀴즈

request 함수 관점에서
서버에서 정상적으로 응답이 온 경우!
특히 유저가 실수 해서 난 Error인 4xx번대 에러인 경우 이를 코드상 에러라고 보는 것이 맞을까요?
아니면 정상적으로 동작하는 것으로 보는게 좋을까요?
만약 정상이라고 본다면 어떻게 처리하는 것이 좋을까요?

throw new Error("API 처리 중 이상 발생");
} catch (e) {
console.log(e.message);
}
};
25 changes: 25 additions & 0 deletions src/core/nestedObjectToArray.js
Copy link
Member

Choose a reason for hiding this comment

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

중첩된 객체를 중접되지 않게 푸는 함수를 생성하셔서 적용시킨 모습이 인상적입니다!! 🤩
다만 데이터가 많아지면 sidebar list를 생성하는데 함수를 한 번 거치는 과정이 필수적이기 때문에 시간적으로 비효율적이라는 생각이 듭니다!!
어떻게 생각하나요? 🤔

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
export default function nestedObjectToArray(nestedObject) {
const nestedObjectArray = [];

function recuresiveNestedObjectToArray(nestedObject, depth = 1) {
nestedObject.forEach((document) => {
if (document.documents.length !== 0) {
nestedObjectArray.push({
id: document.id,
title: document.title,
depth: depth,
});
Comment on lines +4 to +11
Copy link

@nayeon-hub nayeon-hub Jul 10, 2023

Choose a reason for hiding this comment

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

단순 궁금한 점입니다! 함수 내에 함수를 선언해서 사용하신 이유가 있나용? 🤔

recuresiveNestedObjectToArray(document.documents, depth + 1);
} else {
nestedObjectArray.push({
id: document.id,
title: document.title,
depth: depth,
});
}
Comment on lines +6 to +19
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
if (document.documents.length !== 0) {
nestedObjectArray.push({
id: document.id,
title: document.title,
depth: depth,
});
recuresiveNestedObjectToArray(document.documents, depth + 1);
} else {
nestedObjectArray.push({
id: document.id,
title: document.title,
depth: depth,
});
}
nestedObjectArray.push({
id: document.id,
title: document.title,
depth: depth,
});
if (document.documents.length !== 0) {
recuresiveNestedObjectToArray(document.documents, depth + 1);
}

이렇게 작성해도 괜찮을 것 같아요..! 🤔

});
}

recuresiveNestedObjectToArray(nestedObject);
return nestedObjectArray;
}
22 changes: 22 additions & 0 deletions src/core/router.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
const ROUTE_CHANGE_EVENT_NAME = "route-change";

export const initRouter = (onRoute) => {
window.addEventListener(ROUTE_CHANGE_EVENT_NAME, (e) => {
const { nextUrl } = e.detail;

if (nextUrl) {
history.pushState(null, null, nextUrl);
onRoute();
}
});
};

export const push = (nextUrl) => {
window.dispatchEvent(

Choose a reason for hiding this comment

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

오! 멋진 시도네요!!

new CustomEvent(ROUTE_CHANGE_EVENT_NAME, {
detail: {
nextUrl,
},
})
);
};
18 changes: 18 additions & 0 deletions src/core/storage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
const storage = window.localStorage;

export const getItem = (key, defaultValue) => {
try {
const storedValue = storage.getItem(key);
return storedValue ? JSON.parse(storedValue) : defaultValue;
} catch (e) {
return defaultValue;
}
};

export const setItem = (key, value) => {
storage.setItem(key, JSON.stringify(value));
};

export const removeItem = (key) => {
storage.removeItem(key);
};
Loading