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: socket.io infra and decouple ws and chatroom #261

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

ttpss930141011
Copy link
Contributor

@ttpss930141011 ttpss930141011 commented Aug 5, 2023

Why need this change? / Root cause:

這隻 PR & branch 是從這邊分出來的:Here,因為寫一寫忘記切 branch 出來導致弄亂了 @arealclimber 的分支還真不好意思,基本上這支 PR 只是在上面這支 PR 多做一點微調與調整 commit 而已,完整資訊請查閱以上 PR。

  • ... PR 249
  • Moved Socket Constants to file @/contexts/SocketContext
  • Encapsulated connect logic to function to be used flexibly in other components
  • Added console in mock socket server to show online (dev mode) users

Changes made:

  • Follow our codestyle to new useSocketCore.ts .
  • Encapsulate connect logic in SocketProvider.ts as a function.
  • Move Socket some constants to SocketContext.tsx

Test Scope / Change impact:

Issue

@vercel
Copy link

vercel bot commented Aug 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
game-lobby-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 7, 2023 9:23am

@ttpss930141011
Copy link
Contributor Author

ttpss930141011 commented Aug 5, 2023

實測發現只要在 _app.tsx 中塞入 SocketProvider 就會導致 Lighthouse CI 失敗,如圖:
image
同樣情況發生在
bxb 的 ticket: https://github.com/Game-as-a-Service/Game-Lobby-Web/actions/runs/5706761912/job/15462776637?pr=259
areaclimber 的 branch: https://github.com/Game-as-a-Service/Game-Lobby-Web/actions/runs/5771706077/job/15645951559?pr=249

@GaaSBot
Copy link

GaaSBot commented Aug 5, 2023

🤖 Lighthouse Scan Result for 8388796

/rooms
Metric Value
Performance 66
Seo 80
Accessibility 79
HTML Report for LHCI Scan Report Link
/rooms/abc
Metric Value
Performance 75
Seo 80
Accessibility 69
HTML Report for LHCI Scan Report Link
/login
Metric Value
Performance 50
Seo 80
Accessibility 97
HTML Report for LHCI Scan Report Link

@Yuwen-ctw
Copy link
Contributor

Yuwen-ctw commented Aug 6, 2023

Hello Justin @ttpss930141011 ,
個人對這份 PR 有些地方想釐清一下,主要有兩點,以下說明:

  1.  相較於 Shirley 的版本,在 SocketProvider.tsx 中,你將原本放在 useEffect 中有關 Socket 的連線與斷線的兩個邏輯,從 useEffect 抽離為函式,並使用 useCallback 進行記憶,最後再放回 useEffect 裡。
    依現行 PR 的程式碼啟動專案並直接以已登入的身分進入大廳時,會發現 console 區塊多了好幾次連線與斷線的訊息,而進入房間後也無法順利的使用聊天室發送與接收訊息,同時 API 與 WS 的傳輸速度變得很緩慢等等,總之,出現一些非預期的狀況。
     依過往經驗,若 useEffect 執行次數非預期的話,通常是依賴陣列的關係,目前個人的解方是將 connect 函式的依賴陣列將 socket.current 移除。 這樣做之所以有效,個人推測是因為 socket.current 的值在每次 connect 函式執行時都會被重新賦值,而因為重新賦值,所以受依賴陣列的關係讓 connect 函式重新計算為新函式,導致再次觸發 useEffect 。只是如果前述推測正確的話,那為何沒有造成無窮迴圈呢,這是個人現在尚未知曉的。
     另外,disconnect 函式依賴陣列的 socket 之所以不會觸發上述流程,是因為 socket 是使用 useRef 進行管理,因此 socket 這個 useRef 物件內部本身還有一層 current 屬性,此屬性對於 socket 而言始終都不曾改變,因此 useCallback 不會重新計算 disconnect 函式。
     最後,useRef 所管理的變數,是可以選擇不放在依賴陣列理的,因此個人傾向兩函式的依賴陣列都直接設為空陣列,接著再將第 19 行 getEnv 的函式,改放於 SocketProvider 宣告之前進行呼叫,就也可以把 // eslint-disable-next-line react-hooks/exhaustive-deps 這段改變 EsLint 規則的註記給移除了。

  2. 個人對於 Socket 連線與斷線的功能預期,是認為使用者在整個使用平台的期間,只會連線與斷線各一次,且這兩次均由程式自動觸發,使用者是無法主動嘗試觸發的,因此對於現行將 connect 與 disconnect 兩函式從 useContext 對外暴露給元件主動呼叫的功能,暫時無法預期未來會在甚麼情境可以運用,因此想詢問這邊的看法;而這個部分在 Shirley 的版本沒有注意到,因此對當時的 disconnect 也做相同的事情沒有提出詢問,非常抱歉。

以上說明,感謝!

@ttpss930141011
Copy link
Contributor Author

ttpss930141011 commented Aug 6, 2023

Hello @Yuwen-ctw ,
感謝你的回覆!謝謝你第一段提到的相依問題,我的確沒有注意到這樣會導致不可預測的問題,在最新的一次 commit 中有以下改動:

  • getEnv 改放於 SocketProvider 宣告之前進行呼叫
  • 將 useEffect 中依賴項清空
  • 評估過後覺得 connect & disconnect 並不會造成效能問題所以將 useCallback 移除

修改完之後有幾點想討論:

  1. 關於將 connect 與 disconnect 兩函式從 useContext 對外暴露給元件主動呼叫這點,我預想是在進入房間之後才會連上 Socket,並在退出房間後 disconnect,進入下個房間亦然,在非房間情況則是用一般 HTTP/HTTPS 呼叫,這邊比較抱歉的是因為前先陣子比較忙,很少參加到 Socket 的討論,不知預期是只會連線與斷線各一次,這點我在晚點的會議上會提問,以跟上大家進度。
  2. 在 CI 的 LHCI 部分依然會死掉,現在已經將範圍限縮至只要 SocketProvider 中有
   useEffect(() => {
     connect();
     return () => disconnect();
   }, []);

的情況就會失敗,我猜想是在本專案 Lighthouse CI 中,在lighthouserc.js 的 start server command 為 yarn run start,因為是向 server 端進行 socket 請求,因而造成未預期的錯誤,這方面我還想不到如何解決,不知道其他人有什麼想法?

  1. 在 SocketProvider 中有一個 socketStatus 的 State,並只有在 RoomChatroom.tsx 的 useEffect 作為是否有連上 socket 的判斷,但其實 socket 有一個屬性為 connected,能做到這個 socketStatus 差不多的事。所以我將 socketStatus 從 SocketProvider 中拿掉,但發現拿掉之後在 ChatroomProvider 的 sendChatMessage 無法正常發送訊息。
    實測過後只要是在 SocketProvider 中 socket 連結過後 setState 讓 provider re-render 如以下:
// SocketProvider.tsx
socket.current.on(SOCKET_EVENT.CONNECT, () => {
...
setSomeStateHere('something')
...

就能讓 ChatroomProvider 那邊的 socket 取得正常而不會是 undefined,所以我猜想是因為我們使用了 useRef,導致 socket 在連接過後子組件不會因為 socket 改變而 re-render 進而取得正確的值,而 Shirley 剛好在 SocketProvider 裡面有寫了一個 state 誤打誤撞(還是其實有設計好XD) re-render 了 SocketProvider,而 ChatroomProvider 因為 Parent re-render 所以自己也 re-render,然後就取得了正確的 socket。所以我想或許我們這邊改用 useState 會比較好?

以上幾點感謝抽空討論!

@Yuwen-ctw
Copy link
Contributor

Hello Justin,
除了稍早會議提到有關 useEffect 改放回到 SocketProvider 這項調整外,若方便的話也麻煩將測試 CI 的 commits 也一併 squash 掉,以減少 commit 數量,感謝。

- Refactor the code relevant to Websocket (ws) to use socket.io instead.
- Decouple the socket-relevant functionality with ChatroomProvider
- Moved Socket Constants to file `@/contexts/SocketContext`
- Encapsulated connect logic to function
- Added onlineUsers setInterval console in mock socketio server to observer easily
@ttpss930141011 ttpss930141011 force-pushed the refactor/socketIO-infra-justin-version branch from 73c84dc to 914ca06 Compare August 7, 2023 09:22
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@GaaSBot
Copy link

GaaSBot commented Aug 7, 2023

Knip Scan Result for 914ca06

Unused files (10)
Unused files (10)
.bundlewatch.config.js
components/shared/Chat/ChatContent.tsx
components/shared/Chat/ChatMessage.tsx
components/shared/Chat/index.tsx
components/shared/Icon/group/news.tsx
configs/i18nConfigs.ts
lighthouserc.js
reset.d.ts
scripts/knipScanReporter.js
scripts/lhciScanReporter.js
Unused dependencies (2)
Unused dependencies (2)
@svgr/webpack package.json
sharp package.json
Unused devDependencies (5)
Unused devDependencies (5)
@actions/github package.json
@octokit/core package.json
@storybook/blocks package.json
@storybook/testing-library package.json
@total-typescript/ts-reset package.json
Configuration issues (1)
Configuration issues (1)
Unused item in ignoreDependencies: @next/bundle-analyzer

@ttpss930141011
Copy link
Contributor Author

Hello @Yuwen-ctw

在最新的 commit 除了有以下改動:

  1. 已整理合併所有 commit
  2. 將 socket 從 useRef 改寫至 useState
  3. 已經將 useEffect 依賴項更改回來
  4. 僅將 socket 本體從 Provider expose 出去

還有額外在 mock socket server 路由 pages\api\mock\socketio.ts 中五秒 console 一次目前有連線的使用者以方便 debug。
此外 Lighthouse 我在本地瀏覽器的 devtools 中是能跑過的如下圖,所以 CI 跑不過的問題目前依然無解,建議沒有問題的話可以先行合併。
image

@Yuwen-ctw Yuwen-ctw merged commit ee9d91f into main Aug 7, 2023
@Yuwen-ctw Yuwen-ctw deleted the refactor/socketIO-infra-justin-version branch August 7, 2023 10:45
@Yuwen-ctw Yuwen-ctw mentioned this pull request Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor refactor the changes already merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat(infra): websocket - 8
4 participants