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

refactor: navbar에 있는 atom을 제거하고 context를 추가하였습니다 #55

Closed
wants to merge 1 commit into from

Conversation

loopy-lim
Copy link
Collaborator

주요 변경사항

  • navbar에 있는 atom을 제거하고 context를 추가하였습니다

리뷰어에게...

  • 이게 좋은 방법인지는 모르겠지만... jotai를 안쓴다는 관점에서는 좋은 것으로 판단하겠습니다.

관련 이슈

Copy link
Collaborator

@2yunseong 2yunseong left a comment

Choose a reason for hiding this comment

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

저는 jotai가 좋아보이네요. Context는 자식들 모두 불필요한 렌더링을 낳는 것 같아용 (시간적으로 별 차이는 없어보입니다)
개선하려면 memo를 써보는것도..? 채승님이 바꾸겠다고 생각하신 이유가 있나요 ??

main render time (jotai)

image

context

image

Comment on lines +9 to 10
applicationNavbar: { id: number; title: string }[];
className?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ApplicationNavbarState 타입을 사용하는거 어떤가유

@2yunseong
Copy link
Collaborator

Profile log
withContext.json
withJotai.json

@loopy-lim
Copy link
Collaborator Author

@2yunseong
사실 jotai를 덜어내고 싶다라는 생각만이 강해서 그러긴 하는데, ssr과 jotai의 조합은 조금 힘드다 랄까? 그런게 있어서요...
좋네요 memo를 한번 써보고 성능상의 이슈가 해결 안되면 jotai를 ssr에 맞게 잘 써보도록 하는쪽으로 바꾸어볼게요!

@loopy-lim
Copy link
Collaborator Author

loopy-lim commented Feb 23, 2024

@JNU-econovation/econorecruitefe
https://jotai.org/docs/guides/nextjs
이거 괜찮은거 같은데 어떻게 생각하시나요?

@2yunseong
Copy link
Collaborator

오호라 hydrateAtom이라..

Copy link
Collaborator

@geongyu09 geongyu09 left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다!

>
<ApplicationQuestion className="flex-[3_0_0]" />
</ApplicationQuestionsContext.Provider>
</ApplicationNavbarContext.Provider>
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 provider를 app/application/page.tsx에 감싼 이유가 있을까요? 개인적으로 layout.tsx에 모두 넣어도 될 것 같다는 생각이 듭니다! 다른 페이지에서도 똑같이 provider를 만들게 될 텐데, 여기저기 분산되어 있으면 불편할 수 있을 것 같다는 생각이 들었습니다! 물론 다른 주소(다른 라우터)와 공유되지 않고 독립적이라면 큰 불편함은 없을 것 같긴 합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

사실 사용하는 domain?이라고 해야할까요? 그런건 저기밖에 없었어요! ㅋㅋㅋㅋ

Copy link
Collaborator

@bada308 bada308 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

https://jotai.org/docs/guides/nextjs

공식문서를 읽고 이해한 점입니다.

  • atomWithHashatom의 가장 큰 차이는 hydration 시 atom 값이 동기화 되는지 여부.
  • atom은 ssr 시 값이 동기화 되지 않아서 사용 시 어려움이 있다.
  • atomWithHash는 ssr 시 값이 동기화되기 때문에 atom의 문제점을 해결할 수 있다!

이렇게 이해했는데요! 사실 atom + ssr 이 어떤 문제가 있는지 정확히 몰라서 atomWithHash를 사용할 때의 장점이 크게 와닿지 않는 것 같습니다..!

추가적으로 저도 context보단 상태관리 라이브러리를 사용하는 것이 성능측면에서 좋다고 생각합니다.

@loopy-lim
Copy link
Collaborator Author

atom + ssr의 문제점은, 서버에서의 상태와 브라우저의 상태가 다를 수 있다는 것이에요. 이런 문제가 있기 때문에 hydration, dehydration, rehydration이 있는 것이고, 이로 상태를 다시한번 더 동기화를 하고 있어요.
사실 큰 문제는 없지만 나중에 console.log로 찍을때는 분명 1에서 작동하지만, 막상 받아들이는 것은 2일 수 있다라는 비 정상적인 결과가 나올 수 있다고는 알고 있어요.
무론 time과 같은 무조건적으로 다른 경우에는 어쩔수 없이 나온다고 합니다.

@bada308
Copy link
Collaborator

bada308 commented Feb 24, 2024

오 서버 상태와 클라이언트 상태가 다른 게 문제라면 꼭 수정할 필요가 있다고 생각합니다!

context와 jotai의 기능을 사용하는 것 중에선 jotai의 기능을 사용하는 게 좋다고 생각합니다.

@baegyeong
Copy link
Collaborator

@JNU-econovation/econorecruitefe https://jotai.org/docs/guides/nextjs 이거 괜찮은거 같은데 어떻게 생각하시나요?

요 기능 사용해보는 거 좋은 것 같아요!

@loopy-lim
Copy link
Collaborator Author

@JNU-econovation/econorecruitefe
https://jotai.org/docs/extensions/query
보면볼수록... 늘어나는... 문서들....
이건 어떤가요?

@baegyeong
Copy link
Collaborator

@JNU-econovation/econorecruitefe https://jotai.org/docs/extensions/query 보면볼수록... 늘어나는... 문서들.... 이건 어떤가요?

오.. 이런것도 있네요 괜찮은 것 같기두??

@geongyu09
Copy link
Collaborator

https://jotai.org/docs/extensions/query

와우 정말 좋네요! tanstack-query로 받은 데이터를 전역적으로 사용할 수 있다는게 새롭네요! 저는 긍정적인 생각을 가지고 있습니다.

Copy link
Collaborator

@2yunseong 2yunseong left a comment

Choose a reason for hiding this comment

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

오..query를 마치 store처럼 사용하네요?

@loopy-lim
Copy link
Collaborator Author

이제 다만 걱정인 것은, server 상태인지, client상태인지 헷갈릴꺼 같아서 걱정 됩니다.

@loopy-lim
Copy link
Collaborator Author

@JNU-econovation/econorecruitefe
일단 이것은 오래되기도 하고, 관심사가 조금 멀어졌으니 close하고 나중에 다시 다른걸로 하겠습니다.

@loopy-lim loopy-lim closed this Feb 29, 2024
@2yunseong 2yunseong deleted the refactor/36-navbar-atom-remove branch September 9, 2024 05:51
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.

[FE] refactor: Navbar atom을 제거한다.
5 participants