Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
[8팀 최준만] [Chapter 1-2] 프레임워크 없이 SPA 만들기 #2
Changes from 22 commits
37ac031
46c2252
ba6557b
17df315
e398fa5
1c6fc3a
51a666e
0ff32e7
245951e
05b5ecc
e209f2b
96c087f
2e255fc
e26bbb5
0a3d138
11e904a
c7c7541
e5ad353
1765bc8
3db4eae
70f9469
d38022a
20ff127
a1339b7
f5f40fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 제시된
activationLike
를 그대로 사용했는데, 어차피 likeUsers를 전달해주니, 굳이 위에서 체크할 필요가 없었군요..꼼꼼하게 체크하신게 느껴집니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금은 posts가 기본 더미 데이터로 설정되어있긴한데 posts가 비어있다면? 이라는 생각이 들었어요 (저도 사실 고민하다가 Date.now()로 넣어놓고 따로 수정은 안했네요.. ㅎㅎ 더 좋은 방법 있는지 고민해봐도 좋을 것 같습니다)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 store쪽 보시면 id값을 내림차순으로 변경해 놨습니다.
가장 최근에 들어온게 가장 아이디가 크도록요!
저희 과제에선 삭제가 없어서 이렇게 처리했습니다 ㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
20ff127
tc와 함께 구현해봤습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굿 저도 비슷한 생각을 가지고 있는데요!
저는 그래서 위의 코드에서 posts가 비어있을 때 에러가 발생할 수 있을때를 고려한 새 게시물의 id를 추가하는 id를 다음과 같이 추가로 구현해보면 어떨까 싶어요! 제가 지금 코드를 보고 생각한 코드는 다음과 같은데, 추가로 더 고려해보시면 좋을 것 같습니다!
제가 보고 구현해본 내용은 다음과 같은데 테스트 코드는 통과하지만 개인적으로 마음에는 안드는 구조예요 ㅎㅎ,, 준만님께서 더 고민해보시면 좋을 것 같습니다:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정말 비슷한 코드를 원정님 커밋에서 봤는데, 제가 거기에 대안으로 코드를 작성해봤어요. 한번 보시고 이것도 리뷰해 주시죵 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[] 빈배열이 들어가도 1이 나오고, 가장 큰 값에 +1을 더하는 것은 동일합니다.
There was a problem hiding this comment.
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으로 접근하는 방법이 더 안정적일 것 같은데 어떻게 생각히사는지요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤식으로 해야할지 살짝 감이 오지 않습니다.
참고할 커밋 있을까요?
내일 같이 코드리뷰 시간에 얘기해보면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normalizeVNode.js
의vNode.type === "function"
를 통해컴포넌트 함수는 다 처리된 후 createElement로 전달될 것 같은데 혹시 한번 더 체크하시는 이유가 있으신지, 예외케이스가 있으셨는지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오~ 예리하십니다.
저희가 정규화 단계에서 모든 함수 호출을 하기때문에 해당 조건문에 걸릴 일이 없을 것 같네요.
예리한 피드백 감사합니다.
There was a problem hiding this comment.
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"
요런 케이스에 해당하는 요소가 올 경우에 오류가 발생하지 않으셨는지 궁금합니다!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
진짜 문제의 소지가 있겠네요!!
리뷰 감사합니다. 고쳐야될 코드같아요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로는
replacePropIsClass
로 별도의 함수로 뺐다면, 윗줄에서 한 번 실행하고 반환값을 받아서prop
을 인자로 넘겨주는 쪽이 가독성이 더 좋을 것 같아요!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateAttributes 함수를 보면 기존 코드는 모든 속성을 매번 설정하고 있어서 DOM 요소에 이미 설정된 속성이라도 변경 없이도 불필요하게 다시 설정되고 있네요 혹시 맞나용??
제가 개선해보면서 addEvent 함수를 호출하기 전에 이벤트 리스너 속성이 변경되었는지 확인하는 로직까지 한번 추가 해봤는데 요렇게 추가로 해보시는건 어떤가요?? 조금 더 고도화 하면 좋을 것 같습니다 :) 이 함수의 역할이 잘 보이게 간결하게 잘 작성하신 것 같아요!! 좋습니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 코드 개선까지 감사합니다.
여기서 궁금한게,
props[key] !== prevProps[key]
코드에서 핸들러 함수가 상이한 것을 체크할 수 있나요??There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번 과제에서는 따로 사용되지 않아보이는데 코드를 읽어보면 도입하시려는 목적이 DOM 요소들의 자식 요소가 제거되고 나서, 각 제거된 요소에 대해 따로 이벤트 처리를 하시려고 한 것 같네요 어떤 목적으로 구현하려고 하셨나요??? 궁금합니다👀👀
There was a problem hiding this comment.
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을 초기화 하는데 이부분 때문에 이슈가 난다고 판단해서 해당 타이밍을 포착해 다 초기화 해주려고 한 것이었습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 함수는 더이상 사용되지 않는 함수인가요??
처음 들었을때 아이디어는 매우 좋다고 생각했는데 아쉽네요 ㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
디버깅하면서 없앴는데 다시한번 붙여봐야겠네요 ㅋㅋ
There was a problem hiding this comment.
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 인 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
덕분에 최적한 방안에 대해서 생각해 볼 수 있어 좋았습니다. 감사해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventManager에서 중복된 이벤트 이슈를 어떻게 해결해야할지 감이 안잡혔는데, 준만님 코드를 보고 많이 배웠습니다!👍👍👍