-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat/page transition #108
base: main
Are you sure you want to change the base?
Feat/page transition #108
Conversation
수정 사항: 훅이랑 로더 사용 방식을 좀 변경했습니다. |
@joongwon @iihyungsuk @hitkoo @woohm402
|
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.
자세히는 못봤는데 전체적으로 라이브러리가 react-router-dom 이랑 react-query 랑 styled-components 에 강하게 결합된 느낌?
나중에 모종의 이유로 react-router-dom 대신 @tanstack/router 를 쓰고 싶다거나 react-query 를 안 쓰고 싶다거나 styled-components 대신 emotion을 쓰고 싶다거나 등등의 상황이 생길 수도 있고, 다른 프로젝트에서 이 라이브러리를 가져다 쓸 때 기술 스택이 다를 수도 있을 텐데 고럴 때 살짝 아쉬울 수는 있을 것 같아요!
자세히 보려면 완전 각잡고 봐야될거같은데 나중에 시간될때 생각나면 해보겠습니다 ㅋㅋㅋㅋㅋㅋ
url: URL; | ||
}; | ||
|
||
export type PageDataLoader<T extends object> = ( |
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.
export type PageDataLoader<T extends object> = ( | |
export type PageDataLoader<T> = ( |
extends 없어도 되지 않을까요?!
object
는 좋은 타입이 아님- 굳이 T의 타입을 제한할 필요가 있을까?
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.
그러게요 굳이 오브젝트일 필요가 없을듯...?
let isInitialDelay = true; | ||
let lastTimeoutId: null | number | NodeJS.Timeout = null; |
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.
이러면 전역으로 공유돼서 setDelay
를 사용하는 모든 곳에서 동일한 isInitialDelay
랑 lastTimeoutId
를 공유할텐데 의도된건가요?! 의도된건가 아닌가 긴가민가해서 질문
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.
음 어차피 setDelay는 직접 사용하기보다는 page transition관련해서만 사용되고, transition 관련 상태는 단일한 전역 상태여서 저렇게 구현해도 별 문제 없을거라 생각했슴다...!
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.
🤔 넵!
let isInitialDelay = true; | ||
let lastTimeoutId: null | number | NodeJS.Timeout = null; |
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.
number 굳이?
let isInitialDelay = true; | |
let lastTimeoutId: null | number | NodeJS.Timeout = null; | |
let isInitialDelay = true; | |
let lastTimeoutId: null | NodeJS.Timeout = null; |
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.
ReturnType<typeof setTimeout>
어떻습니까
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.
것도좋죠 웹코드인데 NodeJS.
어쩌구 하는거 좀 못생겼긴해요 인정
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.
옹 좋습니다
if (isInitialMount) { | ||
//on initial mount | ||
setIsInitialMount(false); | ||
} else { | ||
// on transition request | ||
if (transitionStatus === "request") { | ||
setIsTransitionActive(true); | ||
} else { | ||
setIsTransitionActive(false); | ||
} | ||
} |
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.
이거 이렇게 하면 첫 마운트 시 setIsInitialMount 가 호출되면서 다시 useEffect 돌면서 두번째 이펙트 수행되지 않나여?
사실상 "첫 마운트 시 리렌더 한번 한다" 랑 비슷한 느낌
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.
사실 지금 isTransitionActive 관련 코드가 다 죽어있습니다. 뭘 하려는지는 몰라도 아직 안 한듯?
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.
- 라이브러리 개발이란 보일러판과 자유도 사이, 마법의 DSL과 친숙한 씬택스 사이에서 외줄을 타는 일이네요. 완성되면 잘 써먹을 것 같습니다.
- react-router와의 커플링 문제는 지금 당장은 크게 신경쓸 필요 없을 것 같아요. 다른 라우터로 바꾼다고 해도 얇게 래퍼 하나 씌우면 되지 않을까요.
- pre-commit은 꺼놓으셨나요? 빌드가 안 되는데요.. 만든 사람이 꺼놓으면 어떡합니까~
cachedResult !== undefined | ||
? cachedResult | ||
: await queryClient.fetchQuery(resultQuery), |
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.
README부터 읽고 있습니다. fetchQuery 함수 자체에서 캐시 동작이 있으니 이런 boilerplate 코드는 빼도 괜찮지 않을까요? 또 예제 코드니까 단순한게 읽기 편하기도 하구요.
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.
제가 react-query를 잘 몰라서 그러는데 fetchQuery만 날려도 자동으로 유효한 캐시값이 있을 때 가져오나요?? 제가 참고한 글에서는 굳이 저렇게 캐시를 확인하는 로직을 넣던데, 만약 fetchQuery로만 다 해결되면 훨씬 간결해질 듯... react-query 정확한 동작을 좀 알아봐야겠습니다.
또 그와 별개로 리드미에 예시 코드를 간결하게 적는 것은 동의... 좀 고쳐보겠습니다
export const homeLoader = createCompositeLoader( | ||
() => () => Promise.resolve({}), // 아무 내용이 없는 dataLoader 함수 | ||
); |
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.
- 초고차함수보다는
(params, context) => Promise
꼴의 함수가 쓰기 편할 것 같습니다. queryClient 처음 받을 때 뭔가 할 일이 있을 것 같지도 않구요. 혹시 카레 중독이신가요? - 아래에 createCompositeLoader를 제대로 소개하는 것을 보니 여기에는
homeLoader = () => () => ...
만 넣어야 하는게 아닌지요?
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.
queryClient는 로더 내부에서 fetchQuery 등을 사용하기 위해 주입하는 것인데, 로더마다 queryClient 임포트해오는 것은 좀 못생긴 거 같아 main.tsx에서 한번에 주입해주려고 초고차함수로 만들었....습니다. 카레 좋잖아요....
// LoaderReturnType을 이용하여 compositeLoader에서 타입을 추출 | ||
export type SomeLoaderReturnType = LoaderReturnType<typeof someCompositeLoader>; | ||
|
||
|
||
//page에서 사용 | ||
export function Page () { | ||
const initialData = usePageData<SomeLoaderReturnType>() | ||
... | ||
} |
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.
initialData = usePageData<typeof loader>()
로 하는 방법도 있습니다. Remix.js에서 이런식으로 썼던 거 같아요. 결국은 design decision이니 참고하시죠.
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.
이거 근데 loader가 Promise를 반환하는 고차함수라 결과값 타입을 뽑기가 좀 더럽더라구요... 물론 LoaderReturnType을 컴포넌트에서 import해 쓸 수도 있긴 함. 그게 간결하려나요?
const animation = usePageAnimation(dashboardMainAnimator); | ||
return <Main $transitionAnimation={animation}>...</Main>; |
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.
usePageAnimation의 파라미터는 항상 일정한 값 내지는 순수함수가 들어가야 하는 거죠? 문서에 해당 조건을 정확하게 써주는 게 좋겠습니다.
아님 animator 값에 따라 다른 animation을 리턴하도록 동작을 바꾸거나요.
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.
애니메이터 부분 코드를 좀 분리할까 싶은데, usePageAnimation 훅의 스펙을 정확히 어떻게 두는 게 나을 지 좀 고민이네요
type HeaderProps = { | ||
isTransitionActive?: 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.
오... 훅을 페이지 컴포넌트에서만 사용하게 두었던 시절의 코드가...
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.
이제 보니 이 파일도 usePageAnimation으로 옮겨갔군요. 옮긴 파일은 좀 지워달라구요~
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.
죄송... 정리하다가 만게 좀 많네요...
useEffect(() => { | ||
setIsInitialMount(false); | ||
}, []); |
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.
이거 애니메이션 끝날 때 콜백으로 넣어줘야하지 않을까요? 애니메이션 도중에 또 페이지 옮기면 복잡해질 것 같은데 아무튼 여기는 아닌 거 같아요. 위에서 현민님이 코멘트 단 것처럼 그냥 마운트할 때 렌더링 두번하는 코드입니다.
|
||
const queryClient = new QueryClient(); | ||
|
||
const router = createBrowserRouter([ | ||
{ | ||
path: "", | ||
element: <Home />, | ||
loader: homeLoader(queryClient), |
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.
이렇게 쓰려고 카레 하셨군요. 그렇더라도 유저는 (param, client) => ...
로 짜게 하고 라우팅 쪽에서 (param) => f(param, client)
로 넣어줄 수도 있겠습니다.. 선택은 라이브러리 주인장의 몫이죠.
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.
음... 그르게요 난 카레가 좋은데...
@@ -39,6 +45,10 @@ export const dashboardLoader = | |||
}; | |||
}; | |||
|
|||
export const dashboardLoader = createCompositeLoader(dashboardDataLoader, { | |||
duration: 500, |
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.
default 값 써도 될 거 같아요~
export const homeLoader = createCompositeLoader( | ||
() => () => Promise.resolve({}), | ||
); |
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.
이럴거면 loader가 필요한가~ 아 애니메이션을 넣어줘야 하네요 되게 귀찮다. dataLoader 파라미터에 기본값 설정하면 어떨까요?
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.
인정 데이터없이 그냥 애니메이션만 얇게 쓰는 경우도 많을 거 같네요
요약
태스크 URL
체크리스트
PR 전
머지 전
작업 목록
화면 전환 애니메이션을 작업해본 좀 큰 PR입니다.
주요 변경 사항
어떻게 구현했는가
(A화면 -> B화면으로 전환 시)
loader를 이용한 이유
참고: react-router - loader
전환 애니메이션 중에, 다음 페이지에서 필요한 데이터를 미리 fetch하여 넘어간 페이지에서는 별도의 로딩 화면 없이 미리 준비된 데이터를 이용하여 화면을 보여줄 수 있게 하기 위해서입니다.
또한
react-router
에서 제공하는 기능을 최대한 살리기 위해 도입해 보았습니다.현재 loader와 react-query를 연동하기 위한 보일러플레이트 코드가 좀 긴데, 간소화하는 작업을 진행할 예정입니다
애니메이션 구현
styled-components의 css 함수를 이용하여 커스터마이징 가능한 애니메이션을 쉽게 생성할 수 있게 만들었습니다. 물론 여전히 코드는 좀 김.
애니메이션의 디버깅을 위하여
animaiton-fill-mode
는 가능하면both
로 두어야 합니다.왜 zustand인가?
크기가 가볍고, 컴포넌트 밖에서 전역 상태 스토어에 접근하는 문법이 간단해 loader에서 animatedTransitionStore에 접근하기가 용이하기 때문입니다.
전역 스토어에서 하는 일이 많지 않다보니 사용하는 라이브러리는 크게 중요하지 않을 것 같습니다.
남은 작업
테스트 방법 (Optional)
yarn
으로 패키지 최신화 후 여러 페이지 돌아다니기기타 질문 및 공유 사항 (Optional)
react-transition-group
과 같은 라이브러리를 사용하거나, 아예 react-router를 덜어내고 다른 라우터를 만들어야 할 것 같습니다. 어떻게 하는 게 좋을까요?/libs
디렉토리에refined-
prefix를 붙인 디렉토리를 만들어 작업할까 하는데, 어떤가요?