Store users in session #231

richpjames · 2024-09-13T08:19:15Z

We need a way of reliably identifying users.
Previously we used the socket's ID as a users ID but a socket ID relates to a users connection not to a user themselves. If a users connection drops momentarily then they will be assigned a new socket ID and kicked out of the game. Some info in the docs here: https://socket.io/docs/v4/server-api/#socketid

In order to address this we followed the approach used in the Socket.IO private messaging guide parts 1 & 2: https://socket.io/get-started/private-messaging-part-2/

Our approach is a bit simpler in that we don't need user -> user communication only client -> server and server -> client communication.

We add a session store server side where we keep our session data. Once we store this session serverside we tell the client to save the session ID too. When the browser connects to the server it checks to see if a session ID exists in sessionStorage, if it does then it uses it to connect to the server, the server then uses it to retrieve the session. If a session ID doesn't exist then a new session is started.

yndajas

Only had time to look through the first commit for now, sorry!

yndajas · 2024-09-17T11:45:06Z

e2e/app.test.ts

@@ -102,6 +99,10 @@ const addName = async (page: Page, name: Player["name"]) => {
 	//   Click join
 	const joinButton = page.getByRole("button", { name: "Join game" });
 	await joinButton.click();
+
+	//   Verify colour is visible
+	await page.getByText("Connected 🟢");


Could we maybe consolidate connect and addName? connect no longer connecting is a bit misleading

yndajas · 2024-09-17T11:46:51Z

client/index.ts

@@ -45,7 +45,7 @@ const generateSocketUrl = (): string => {
 const socket: Socket<
 	ClientboundSocketServerEvents,
 	ServerboundSocketServerEvents
-> = io(generateSocketUrl());
+> = io(generateSocketUrl(), { autoConnect: false });


Is it worth adding a brief explanation in the commit message of why we're moving towards having 'sessions'? I.e. to allow users to reconnect after refreshing their page/network issues etc?

It might also be useful to link to the guide that we followed for this, or some docs that explain all the socket.auth/socket.handshake.auth/socket.data stuff

yndajas · 2024-09-17T11:49:53Z

client/utils/socketUtils.ts

@@ -7,9 +7,11 @@ import type {

 const addPlayer = (
 	socket: Socket<ClientboundSocketServerEvents, ServerboundSocketServerEvents>,
-	name: string,
+	username: string,


The Player type isn't updated to use username in this commit, so I think this might introduce multiple names for the same thing?

I'd be tempted to leave it as name until/unless we start enforcing uniqueness, although I can see the issue here:

socket.data.userId = randomId(); socket.data.username = username;

Another option:

socket.data.user = { id: randomId(), name: }

yndajas · 2024-09-17T11:54:03Z

server/socketServer.ts

@@ -1,3 +1,4 @@
+import crypto from "node:crypto";


It looks like Bun implements this, so we should be able to drop the import

yndajas · 2024-09-17T11:54:27Z

server/socketServer.ts

@@ -10,6 +11,8 @@ import { Lobby } from "./models/lobby";
 import { Round } from "./models/round";
 import { logWithTime } from "./utils/loggingUtils";

+const randomId = () => crypto.randomBytes(8).toString("hex");


And then is there a reason for not using a UUID here, e.g. crypto.randomUUID()? That could probably just be used inline below without creating a wrapper method

yndajas · 2024-09-17T12:05:58Z

server/socketServer.ts

 		this.server.on("connection", (socket) => {
-			logWithTime(`Socket connected: ${socket.id}`);
+			logWithTime(`Socket connected: ${socket.handshake.auth.username}`);


We should update socket.on("disconnect" too, and maybe include one of session ID and user ID since the name might not be unique (at least for now)?

logWithTime( "User connected", [ `Name: ${socket.data.username}`, `ID: ${socket.data.userId}` ].join("\n") )

yndajas · 2024-09-17T12:15:21Z

server/socketServer.ts

+			socket.data.sessionId = randomId();
+			socket.data.userId = randomId();


In this commit I'm not too clear on how user ID and session ID will be used differently. Are they both needed? Is it worth explaining in the commit message?

This will allow us to fetch the session later by accessing the socket.data object in the server or client. We no longer autoconnect but only connect once a user has submitted their name. This prevents users being added to the session before they have comitted to the game. This allows users to retain their gamestate if their connection drops for any reason. This is following the guidance outlined here: https://socket.io/get-started/private-messaging-part-1/ Co-authored-by: Ynda Jas <ynda@dxw.com> Co-authored-by: George Eaton <george@dxw.com> Co-authored-by: Liz Daly <liz.daly@dxw.com>

We want this logic in two places - when a player clicks join game (listening to the 'players:post') event and when restoring session of a previously joined player. It makes sense to abstract it to a function. I think it would be good if we extracted all event handlers like this to improve readability.

This will be used to store sessions serverside. This could be replaced by something like Redis or SQLite in future but this class is good enough for now. We save the session on connection and disconnection so it should always be up to date

We need a way for users to reconnect to their serverside session if their connection drops or they leave the page and return. We use sessionStorage so that the session is not shared across tabs as it would be for localStorage. We receive the "session:set" event from the server which tells us the server has save the session and now it's time for the client to do the same. We use the "DOMContentLoaded" event to attempt to read the sessionId from localStorage, if it exists then we connect. If it doesn't exist the user will have to manually connect by entering their name. Co-authored-by: Ynda Jas <ynda@dxw.com> Co-authored-by: George Eaton <george@dxw.com> Co-authored-by: Liz Daly <liz.daly@dxw.com>

Now we have a reliable way of tracking users we can use it across the app. Co-authored-by: Liz Daly <liz.daly@dxw.com> Co-authored-by: Rich James <rich.james@dxw.com> Co-authored-by: George Eaton <george@dxw.com>

richpjames force-pushed the store-users-in-session branch 6 times, most recently from edde8c5 to 5f88793 Compare September 17, 2024 10:42

richpjames marked this pull request as ready for review September 17, 2024 10:42

yndajas reviewed Sep 17, 2024

View reviewed changes

richpjames force-pushed the store-users-in-session branch 2 times, most recently from 7b955f5 to c166342 Compare September 18, 2024 08:21

Gweaton and others added 5 commits September 18, 2024 09:29

Add a session storage

0d1e018

This will be used to store sessions serverside. This could be replaced by something like Redis or SQLite in future but this class is good enough for now. We save the session on connection and disconnection so it should always be up to date

Replace socketId with sessionId

d4f38c7

Now we have a reliable way of tracking users we can use it across the app. Co-authored-by: Liz Daly <liz.daly@dxw.com> Co-authored-by: Rich James <rich.james@dxw.com> Co-authored-by: George Eaton <george@dxw.com>

richpjames force-pushed the store-users-in-session branch from c166342 to d4f38c7 Compare September 18, 2024 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store users in session #231

Store users in session #231

richpjames commented Sep 13, 2024 •

edited

Loading

yndajas left a comment

yndajas Sep 17, 2024

yndajas Sep 17, 2024

yndajas Sep 17, 2024

yndajas Sep 17, 2024

yndajas Sep 17, 2024

yndajas Sep 17, 2024 •

edited

Loading

yndajas Sep 17, 2024 •

edited

Loading

		socket.data.sessionId = randomId();
		socket.data.userId = randomId();

Store users in session #231

Are you sure you want to change the base?

Store users in session #231

Conversation

richpjames commented Sep 13, 2024 • edited Loading

yndajas left a comment

Choose a reason for hiding this comment

yndajas Sep 17, 2024

Choose a reason for hiding this comment

yndajas Sep 17, 2024

Choose a reason for hiding this comment

yndajas Sep 17, 2024

Choose a reason for hiding this comment

yndajas Sep 17, 2024

Choose a reason for hiding this comment

yndajas Sep 17, 2024

Choose a reason for hiding this comment

yndajas Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

yndajas Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

richpjames commented Sep 13, 2024 •

edited

Loading

yndajas Sep 17, 2024 •

edited

Loading

yndajas Sep 17, 2024 •

edited

Loading