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

#114 [bug] Express & Socket.io sessions are not synchronized automatically #117

Conversation

withSang
Copy link
Member

@withSang withSang commented Aug 17, 2022

Summary

It fixes #113
src/route/chats.socket.js에서 Promise constructor에 async 함수가 들어가는 패턴의 chatsForRoom 함수를 Promise constructor 없이 똑같은 기능을 하도록 수정하였습니다.

It fixes #114
Socket.io에서 사용자 session에 chatRoomId 속성을 설정해도 express의 session에 해당 값이 바로 저장되지 않는 문제가 발생하여, session.save()를 호출하여 명시적으로 Socket.io 세션과 express의 세션을 동기화했습니다.

Extra info

  • Needs more than 2 reviewers? y
  • Needs more than 10 minutes for review? y
  • Needs to execute in order to review? y

Images

화면 기록 2022-08-18 오전 3 46 46

  • 수정 후, 브라우저에서 채팅방에 처음 들어갔을 때에도 사진이 잘 전송되었습니다. 수정한 코드를 주석 처리하면 문제가 다시 발생합니다.

Further Work

  • /static 라우터 제거: 서버에서 정적 파일을 호스팅할 필요가 없어졌습니다.
  • 개발용 /rooms, /users API 제거: admin 페이지로 대체 가능합니다. Add: support maxPartLength in /rooms/search #100 (comment)
  • auth.replace 라우터 테스트케이스 고치기: 로그인 시도 url이 /auth/try여야 하는데 테스트 코드에서는 /auth/sparcssso로 적혀 있어서 로그인 성공 여부가 제대로 테스트되지 않는 문제가 발생했습니다. 어제 발생한 [Bug] auth.replace 라우터 새 유저 생성 안됨 #115 도 CI 단계에서 걸렸었어야 했는데 테스트 코드를 잘못 짜서 그러지 못했어요 ㄷ

@withSang withSang added 😱 bug Something isn't working chatting labels Aug 17, 2022
@withSang withSang self-assigned this Aug 17, 2022
@withSang withSang linked an issue Aug 17, 2022 that may be closed by this pull request
1 task
@withSang withSang changed the title #114 bug express socketio sessions are not synchronized automatically #114 [bug] Express & Socket.io sessions are not synchronized automatically Aug 17, 2022
Copy link
Member

@14KGun 14KGun left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@GrasshopperBears GrasshopperBears left a comment

Choose a reason for hiding this comment

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

👍

@withSang withSang merged commit 29e00d8 into main Aug 19, 2022
@withSang withSang deleted the #114-bug-express-socketio-sessions-are-not-synchronized-automatically branch August 19, 2022 09:50
Copy link
Member Author

@withSang withSang left a comment

Choose a reason for hiding this comment

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

저걸 주석처리하면 작동이 안 된다는 걸 확인하려고 주석처리해놨는데 그대로 머지했어요..
죄송합니다 😢

@@ -118,20 +113,24 @@ const ioListeners = (io, socket) => {
// join chat room
joinChatRoom({ session: session }, socket.id, roomId);
socket.join(`chatRoom-${roomId}`);
// session.save(); // Socket.io 세션의 변경 사항을 Express 세션에 반영.
Copy link
Member Author

Choose a reason for hiding this comment

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

@14KGun @GrasshopperBears @ptptsw
으악 제가 여기 주석 처리해놨어요
다시 커밋해야겠어요.. 죄송합니다 ㅠㅠ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😱 bug Something isn't working chatting
Projects
None yet
3 participants