[Refactor] useWebsocket 리팩토링 #317
Conversation
There was a problem hiding this comment.
고생하셨습니다!
ingame소켓 로직이 분리가되어서 팀원들 모였을때 한번 테스트 해보고 approve하면 좋을 것 같아요
반환하는 함수가 많아져서 위에 적은 것처럼 publish함수를 통일했는데, 길어지더라도 기존처럼 각각함수를 만들어두고 반환하는게 좀더 가독성이 좋을까요?
만약 기존처럼 한다면 : 호출하는데서 handlePubReadyGame() 이렇게만 하면 되서 이해가 쉽고 엔드포인트 입력시 휴먼에러를 줄일 수는 있겠네요..
추상화를 한 단계 낮추신 것 같아요. 예를들어 api를 제작했을때 getTodos(...)라는 get요청을 한번 래핑한 함수가 있을테고 이 함수는 내부적으로 axios.get이나 axios.get을 래핑한 어떠한 get함수를 사용했을 것 같아요.
삭제하신 로직은 엄밀히 말해 API와 다르지만, getTodos(...)와 래핑 단계가 비슷하다고 보여지고 추가하신 부분은 axios.get이나 axios.get을 래핑한 어떠한 get함수라고 보여지네요
API단계에서도 둘은 취향차이인 것 같습니다. 다만 휴먼에러를 줄일수 있다는 점에 공감이 가네요
그리고 useWebosket 외부에서 publish에 인자를 동적으로 넘겨주어야 한다면 이번 pr처럼 수정하신 게 맞다고 생각이 듭니다!
저는 이전 방식이 쪼금더 가시성이 좋은 것 같아요
웹소켓 관련하여 어떤 개선이 더 있으면 좋을까요?
코드리뷰에 남겨두었습니다!
| stompClient: MutableRefObject<Client | undefined>; | ||
| ingameSubscription: MutableRefObject<StompSubscription | undefined>; | ||
| } | ||
| const useIngameWebsocket = ({ |
There was a problem hiding this comment.
IngameWebosocket으로 분리된 건 좋은것 같아요
여기서 조금 더 욕심을 내서 useWebsocket을 말 그대로 웹소켓을 이용하는 훅으로 만들고 waitingroom과 ingame에서 useWebsocket을 활용하는 방법은 어떠신가요?
저도 useSse를 위와 같은 방식으로 리팩토링하고싶은데 꽤 어렵더라구요 😲
There was a problem hiding this comment.
useWebsocket의 추상화가 낮춰질 것 같은데, 이러면 인게임과 대기실은 다루는 기능이 달라서 추가적인 기능이 생길 때 수정할 사항이 많이 생길 것 같은데 어떻게 생각하시나요?! 🤔
There was a problem hiding this comment.
네 저두 말씀주신것처럼 이름대로 websocket훅이다보니 이 훅 안에선 객체를 만들어 연결하고 구독하는 것 까지 하고 그객체를 반환해보자가 목표였는데,
ingame쪽만 분리되고 waitingroom이 애매했네요 🫠
useIngameWebsocket에 문제없는거 확인하고 이어서 해봐야겠어요 !
There was a problem hiding this comment.
오히려 기능이 달라서 각각을 구별지어야겠다고 생각했어요 ..!
대기방에서 채팅용 웹소켓 연결도 생기는걸 생각하면 딱 연결역할을하는 훅을 두고, 그위에 추가되는 연결을 하는?!!
그리구,
useWebsocket의 추상화가 낮춰질 것 같은데
요 말씀을 제가 제대로 이해를 못한 것 같은데 조금 설명부탁드려도 될까용
There was a problem hiding this comment.
websocket 훅 내부에서 웹소켓 연결 하고 구독하는 것은 좋아보입니다!
그런데 로직이 아니라 객체를 반환하려고 하는 이유가 궁금합니다!! 🤔
전체적인 구상이 잘 이해가 안가서 그러는데
- useWebsocket 훅 하나만 살리는 방향으로 진행하는 걸까요? 아니면
- useWebsocket, useIngameWebsocket 훅 2가지를 다 살리는 방향인가요?
- 추가되는 연결 이 publish 관련된 로직을 말하는 것 일까요?!?! 🤔
useWebsocket의 추상화가 낮춰질 것 같은데
제가 말을 잘못 이해해서 이렇게 말을 했었네요 ㅠㅠ
어떤 맥락에서 얘기했었냐면
현재 useWebsocket 내부에서 ingame과 waitingroom에 관련된 로직들(추상화가 구체적)을 만들어서 반환하는데 이 로직들을 ingame과 waitingroom에서 공통적으로 쓸 수 있게 만들려면 범용적으로 만들어야 해서 추상화가 낮춰질 것 같다는 얘기였습니다!
There was a problem hiding this comment.
@dlsxjzld
우선, 객체로 반환하려고 한 이유는, 인게임 관련 로직을 분리하고 싶어서 였습니다!
한 게임방에 대해 웹소켓객체(client)를 만들고 -> 그 위에 인게임 웹소켓까지 연결하기 위해서 stompClient라는 Ref객체를 두어서 참조했었잖아요!
이 작업 모두가 useWebsocket 한 파일에 있었는데, 여기서 인게임 관련 로직들을 다룰 훅에 게임방웹소켓을 전달해주기 위해 반환했어요!
useWebsocket 훅 하나만 살리는 방향으로 진행하는 걸까요? 아니면
useWebsocket, useIngameWebsocket 훅 2가지를 다 살리는 방향인가요?
두개 다 존재합니다!
There was a problem hiding this comment.
웹소켓 분리 좋은 것 같습니다! 😊
반환하는 함수가 많아져서 위에 적은 것처럼 publish함수를 통일했는데, 길어지더라도 기존처럼 각각함수를 만들어두고 반환하는게 좀더 가독성이 좋을까요?
만약 기존처럼 한다면 : 호출하는데서 handlePubReadyGame() 이렇게만 하면 되서 이해가 쉽고 엔드포인트 입력시 휴먼에러를 줄일 수는 있겠네요..
- 다 장단점이 있는 것 같습니다! 추상화를 구체적으로 하면 휴먼에러를 줄일 수 있는 장점이 있는 것 같고, 단순하게 하면 범용적으로 쓸 수 있는 장점이 있는 것 같아요 ㅎㅎ 어떤 것을 선택하든 통일시키는 구조로 가면 될 것 같습니다!! 👍
| @@ -7,28 +7,18 @@ import useGameWaitingRoomStore from '@/store/useGameWaitingRoomStore'; | |||
| import useIngameStore from '@/store/useIngameStore.ts'; | |||
| import { checkIsEmptyObj } from '@/utils/checkIsEmptyObj'; | |||
| import { getToken } from '@/utils/getToken'; | |||
| import { PayloadType } from '../types/websocketType.ts'; | |||
|
|
|||
| const useWebsocket = (roomId: number | null) => { | |||
| const stompClient = useRef<Client>(); | |||
| const ingameSubscription = useRef<StompSubscription>(); | |||
There was a problem hiding this comment.
요거는 ingame 관련된 거라 useIngameWebsocket으로 가야할 것 같아요!! 👍
| publishIngame, | ||
| publishGameRoom, | ||
| stompClient, | ||
| ingameSubscription, |
There was a problem hiding this comment.
요 부분도 위와 마찬가지로 useIngameWebsocket으로 가면 될 것 같아요 ㅎㅎ
📋 Issue Number
close #316
💻 구현 내용
ex) 기존 handlePubStartGame() => 수정 publishGameRoom('/start');📷 Screenshots
🤔 고민사항
만약 기존처럼 한다면 : 호출하는데서 handlePubReadyGame() 이렇게만 하면 되서 이해가 쉽고 엔드포인트 입력시 휴먼에러를 줄일 수는 있겠네요..