-
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
[#132] 대회 페이지 내의 웹소켓 연결 상태를 확인할 수 있는 기능 #143
The head ref may contain hidden characters: "132-\uB300\uD68C-\uD398\uC774\uC9C0-\uB0B4\uC758-\uC6F9\uC18C\uCF13-\uC5F0\uACB0-\uC0C1\uD0DC\uB97C-\uD655\uC778\uD560-\uC218-\uC788\uB294-\uAE30\uB2A5"
Conversation
❌ Deploy Preview for algo-with-me failed.
|
export default function Loading({ size, color }: Props) { | ||
return ( | ||
<svg | ||
xmlns="http://www.w3.org/2000/svg" | ||
width={size} |
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.
그냥 궁금해서 그런데, 혹시 { size, color } 대신 props를 작성한 뒤, width={props.size} 와 같이 작성하지 않으신 이유가 있으신가요?
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.size로 사용하진 않고
function Loadin(props){
const {size,color} = props
}
or
function Loadin({size,color}){
}
둘 중 하나를 쓰더라구요
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.
이렇게 사용할 생각은 한 번도 못 해봤는데, 덕분에 배워갑니다 감사합니다:D
const ONE_MIN_BY_SEC = 60; | ||
const ONE_HOUR_BY_MIN = 60; | ||
const ONE_HOUR_BY_SEC = ONE_HOUR_BY_MIN * ONE_MIN_BY_SEC; |
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.
이렇게 작성한 거 정말 좋은 것 같아요:D
const wrapperStyle = css({ | ||
position: 'relative;', | ||
}); | ||
|
||
const disConnectedStyle = css({ | ||
color: 'darkred', | ||
}); | ||
|
||
const loadingBoxStyle = css({ | ||
display: 'flex', | ||
gap: '1rem', | ||
}); | ||
|
||
const positionRightStyle = css({ | ||
display: 'flex', | ||
position: 'absolute;', |
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.
엇 그렇네요.
다른 곳에서 쓰던 습관이..
여담인데, panda css는 css 작업할 때 자동완성 기능이 거의 없는 것 같아서 좀 아쉬워요.
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 delayFactor = 2000; | ||
setInterval(() => { | ||
console.log('ping 5초( + 네트워크 지연) 마다 실행'); | ||
const serverTime = new Date(); | ||
handlePingMessage(serverTime); | ||
}, 5000 + Math.random() * delayFactor); |
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.
delayFactor는 웹 소켓 연결시에 딜레이가 생길 걸 예상해서 작성하신 건가요?
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.
웹 소켓이 연결된 상태에서 ping을 보내는 거라 연결시에 딜레이라기 보단, 네트워크 지연 시간입니다. 클라이언트와 서버 사이에 전달하는 동안 걸리는 시간? 정도입니다.
r="35" | ||
stroke-dasharray="164.93361431346415 56.97787143782138" | ||
> | ||
<animateTransform |
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.
처음보는 SVG 태그네요. SVG는 애니메이션을 이런식으로 구현하고 있었군요
endsAt: string; | ||
} | ||
|
||
export default function Time(props: Props) { |
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.
컴포넌트 이름을 Timer로 해주세요!
여담이지만 현재 저희 린트가 잡아주는 룰이 아니라서 나중에 여유가 되면 이런 것도 강제할 수 있는 룰이 있는지 찾아봐야겠네요
|
||
let timerIntervalId: NodeJS.Timeout; | ||
|
||
export default function useConnectHeader({ socket, endsAt }: UseConnectHeader) { |
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.
이름 useTimer로 바꿔주세요!
let timerIntervalId: NodeJS.Timeout; | ||
|
||
export default function useConnectHeader({ socket, endsAt }: UseConnectHeader) { | ||
const endTime = useMemo(() => new Date(endsAt).getTime(), [endsAt]); |
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.
여기는 그냥 endsAt 말고 end Date 객체를 받는게 어떨까요?
상위에서 Date 객체를 내려주는게 이 훅에서 Date가 어떤 형식으로 서버에서 내려오는지에 대해 신경쓰지 않아도 되고, Date를 다루기도 용이하다고 생각합니다.
그리고 useMemo를 쓴 이유가 궁금해요
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.
대회가 끝날 때의 정보가 어디서 오는지 찾다가
type CompetitionInfo = {
id: CompetitionId;
name: string;
detail: string;
maxParticipants: number;
startsAt: string;
endsAt: string;
createdAt: string;
updatedAt: string;
};
에서 오더라구요. 상위에서 endsAt가 string으로 와서 그대로 string으로 받고 있습니다.
new Date(endsAt)해서 데이트 객체로 넘겨주는 게 범용성 있긴 하네요. 좋은 관점 감사합니다.
useMemo는 끝나는 시간인 endTime이 endsAt가 변경 사항이 없다면 다시 연산되지 않았으면 해서 useMemo 사용 했습니다.
endsAt: string; | ||
} | ||
|
||
let timerIntervalId: NodeJS.Timeout; |
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.
interval은 useRef로 감싸서 관리하는게 어떤가요. 물론 그럴일은 없지만, 전역적으로 id를 하나 두고 관리하면 useTimer 훅을 다른 곳에서 재사용하게 될 때 문제가 생길 것 같습니다.
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.
W😎W !
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 handlePingMessage = useCallback((time: Date | string) => { | ||
clearInterval(timerIntervalId); | ||
time = typeof time === 'string' ? new Date(time) : time; |
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.
여기서 Date 객체를 받는건 실제 소켓이 구현된 뒤에도 동일한가요?
정해진 명세가 있다면 그 명세대로 작성되는게 좋아보이긴해요
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.
구두 상으로 Date 받기로 했는데, 스웨거 api를 보면 Date 관련 프로퍼티들 타입이 모두 string이라
string이면 new Date로 감싸고 Date 객체면 그대로 Date를 쓰도록 삼항연산자를 썼습니다.
|
||
export default function useConnectHeader({ socket, endsAt }: UseConnectHeader) { | ||
const endTime = useMemo(() => new Date(endsAt).getTime(), [endsAt]); | ||
const [remainTime, setRemainTime] = useState<number>(-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.
remainTime임에도 숫자형인 것은 살짝 햇갈릴여지가 있어보여요. Date로 다루는 편이 편하지 않을까요?
frontend/src/pages/ContestPage.tsx
Outdated
@@ -117,7 +119,7 @@ export default function ContestPage() { | |||
</div> | |||
</section> | |||
<section> | |||
<SubmissionResult socket={socket.current}></SubmissionResult> | |||
<SubmissionResult socket={socket.current} endsAt={endsAt}></SubmissionResult> |
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.
제출 결과를 출력하는 곳에서 endsAt을 받는 것은 다소 어색해요.
그리고 제출까지 남은 시간은 여기가 아니라 header 같은 곳에서 출력해야하지 않을까요
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.
컴포넌트 분리했습니다 !
frontend/src/utils/date/index.ts
Outdated
export const formatTimeFromMiliSeconds = (ms: number) => { | ||
const sec = Math.floor(ms / ONE_SEC_BY_MS); | ||
// 시간(초)을 'hh:mm:ss' 형식으로 변환 | ||
const hours = Math.floor(sec / ONE_HOUR_BY_SEC); | ||
const minutes = Math.floor((sec % ONE_HOUR_BY_SEC) / ONE_MIN_BY_SEC); | ||
const seconds = sec % ONE_MIN_BY_SEC; | ||
|
||
return [hours, minutes, seconds].map((time) => String(time).padStart(2, '0')).join(':'); | ||
}; |
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.
formatDate 유틸이 이미 있으니 formatDate를 확장해서 쓰는게 어떨까요?
if(form === 'hh:mm:ss') {
...어쩌고
}
그리고 이 함수는 테스트가 작성되어 있는데, 이참에 테스트까지??? (이건 그냥 추가적인 도전과제로 봐주시면 좋겠습니다 :) )
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.
초마다 -1000(ms) 하면서 시간을 찍고 있는데, Date 객체를 직접 연산하는 것이 불편해서
formatTimeFromMiliSeconds( 현재 밀리초(ms) - 1000(ms)) 로 계산하고 있습니다.
input이 하나는 Date 객체고 하나는 ms라서 다르게 사용하고 있는데 ms를 Date객체로 변경해서 넘겨준다면 하나의 함수로 사용할 수 있을 것 같은데. 지금은 그대로도 좋아보입니다. 어떻게 생각하시나요?
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.
저는 Date 객체로 통일하는 편이 좋아보이긴해요. format 함수(즉, 날짜를 나타내는 문자열을 만드는 함수)가 받는 인자가 number가 되어버리면 살짝은 어색해보여요. 물론 ms라고 표기해서 millisec라는 것을 알 수 있지만, 타입에서 인자가 날짜를 나타내는 정보임이 드러나면 더 좋을 것 같습니다.
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.
아 Time이 아니라 TimeGap 이라서 ms를 사용하려고 하는거군요
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.
그럼 이름을 formatMilliSecond(form: 'hh:mm:ss') 같은 이름을 쓰는건 어때요
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.
formatMilliSecond(form: 'hh:mm:ss') 완전 맘에 듭니다.
|
||
import Loading from '@/components/Common/Loading'; | ||
|
||
export default function Connection() { |
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.
이름 일치!
그리고 Disconnection보단 Connecting 인 것 같고, 저 같으면 ConnectState같은 이름으로 만들고 연결, 연결 끊어짐, 연결 중 상태를 나타낼 수 있는 컴포넌트로 만들 것 같기는 해요
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.
연결 끊어지면 sockeit.io에서 바로 연결 요청을 하니 연결 끊어짐과 연결 중은 구별할 수 없을 것 같고,
- 연결 끊김과 연결
- 연결 중과 연결
으로 나누는 것도 좋아보이는데, 연결을 사용자에게 보여줘야 할 지는 좀 의문입니다. 프로그래머스는 끊김만 보여주는 걸로 기억해요.
이름은 변경하겠습니다. 네이밍을 계속 변경하니 체크해도 자꾸 틀리네요. 더 주의하겠습니다.
@@ -0,0 +1,54 @@ | |||
import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; | |||
|
|||
import { Socket } from 'socket.io-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.
utils/socket에서 Socket 타입을 반환하고 있으니 그걸 사용해주세요. 너무 상위에서 socket.io-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.
헉. 자동완성이..
타입도 utils로 빼는건 다시 봐도 좋은 것 같습니다 !
<section className={loadingBoxStyle}> | ||
<span className={disConnectedStyle}>연결 중...</span> | ||
<Loading color="darkred" size="24px" /> | ||
</section> |
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.
여기는 Disconnection과는 다른건가요?
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.
넵, 저건 Timer라고 아예 분리했습니다.
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.
원래
connection안에 연결 끊기면 disconnection or timer였는데
timer를 대회 문제 헤더 ? 쪽에 위치하게 했습니다.
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(() => { | ||
// TODO time 0이면 대시보드로 이동하는 로직 | ||
// 해당 PR에서 해결할 문제는 아니라 PASS | ||
if (Math.floor(remainMiliSeconds / 1000) <= 0) { | ||
// 나가는 로직 | ||
} | ||
}, [remainMiliSeconds]); |
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.
이건 리뷰라기보단 질문인데, 이 부분에 time 0일때 어떻게 처리할지를 외부로 분리할 수 있나요?
만약 그게 된다면 대회 세부정보 페이지에서 재활용이 가능할 것 같아서요
(대회 시작 시간이 되면 대회 입장 버튼 활성화하는 부분)
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.
오! 좋네요. useTimer 사용하고 시간 0초 됐을 때 입력받은 콜백 함수를 수행한다? 좋은 것 같습니다. useTimer(cb) 하고 저 // 나가는 로직에 cb() 호출만 하면 되겠네요!
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.
LGTM:D
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.
의문이 드는건 common에 있기엔 의존성이 너무 많은 컴포넌트 같습니다.
사용하는 hooks도 hooks/common/useTimer 이런식으로 해야할까요?
아니면 그냥 components/Timer로 분리하는 게 더 좋아보이네요.
변경하겠습니다.
export default function Timer(props: Props) { | ||
let { socket, endsAt, isConnected } = props; | ||
// api 연결이 X endsAt 대신 임시로 만들어놓은 것. |
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.
isConnected는 사용되진 않지만 미래를 생각해서 넣은걸까요?
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.
밑에서 사용되고 있습니다
isConnected && remainMiliSeconds !== -1 ? <시간> : <로딩>
알아보기 힘든것 같아서
if (isConnected && remainMiliSeconds !== -1) {
return (
<section className={wrapperStyle}>
<div>
<span className={timeTextStyle}>{formatMilliSecond(remainMiliSeconds, 'hh:mm:ss')}</span>
</div>
</section>
);
}
return (
<section className={wrapperStyle}>
<div>
<section className={loadingBoxStyle}>
<span className={disconnectedStyle}>연결 중...</span>
<Loading color="darkred" size="24px" />
</section>
</div>
</section>
);
}
로 분기처리 했습니다.
<span className={timeTextStyle}>{formatMilliSecond(remainMiliSeconds, 'hh:mm:ss')}</span> | ||
) : ( | ||
<section className={loadingBoxStyle}> | ||
<span className={disConnectedStyle}>연결 중...</span> |
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.
disconnect는 하나의 단어여서 disconnectedStyle이 맞는 것 같습니다
{!props.isConnected && ( | ||
<div className={rowStyle}> | ||
<span>연결 중...</span> | ||
<Loading color="darkorange" size="24px" /> | ||
</div> | ||
)} |
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(!props.isConnected) return null;
return <> ... </>
이렇게 코드를 작성해주실 수 있나요?
단순해서 상관 없지만 제 개인적인 취향이라 요청드리는거고, 꼭 고쳐야하진 않습니다.
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.
항상 html 태그를 리턴해야한다고 생각했는데, null도 되네요.
export const formatMilliSecond = (ms: number, form: string) => { | ||
const sec = Math.floor(ms / ONE_SEC_BY_MS); | ||
|
||
if (form === 'hh:mm:ss') { | ||
// 시간(초)을 'hh:mm:ss' 형식으로 변환 | ||
const hours = Math.floor(sec / ONE_HOUR_BY_SEC); | ||
const minutes = Math.floor((sec % ONE_HOUR_BY_SEC) / ONE_MIN_BY_SEC); | ||
const seconds = sec % ONE_MIN_BY_SEC; | ||
return [hours, minutes, seconds].map((time) => String(time).padStart(2, '0')).join(':'); | ||
} | ||
return ''; | ||
}; |
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.
👍
이건 여담이지만, 커링이라는 기술이 있어요. (여기서 적용해달라는건 아니고 그냥 알려드리는겁니다)
커링을 적용하면 다음과 같이 사용할 수 있습니다.
formatMilliSecond(대충ms, 'hh:mm:ss') // 'hh:mm:ss' 형태의 문자열
const format대충ms = formatMilliSecond(대충ms); // 포멧팅을 할 수 있는 함수
format대충ms('hh:mm:ss'); // 'hh:mm:ss' 형태의 문자열
어떻게 하면 이렇게 동작할 수 있을까?, 이렇게하면 뭐가 좋을까? 이런점을 고민해보시면 재밌을 것 같네요 :)
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.
:)
한 일
연결 상태 확인할 수 있는 컴포넌트 생성
컴포넌트에 필요한 state 일단 props로 전달
소켓이 연결됐고, 서버 시간이 도착했다면 남은 시간을 보여줌
소켓이 연결 안 됐거나, 서버 시간이 도착하지 않았다면 연결중...을 보여줌
참고사항
2023-11-28.3.28.18.mov
UI 변경사항
2023-11-28.5.08.09.mov