Clustered WebSockets + Run session refactor#1511
Conversation
b0b65c7 to
b439a6b
Compare
| ): Promise<WsResponse<unknown>> { | ||
| const userID = client.userId; | ||
| const leaderboardData = { | ||
| mapID: data.mapID, | ||
| gamemode: data.gamemode, | ||
| trackType: data.trackType, | ||
| trackNum: data.trackNum | ||
| }; | ||
|
|
||
| if (!(await this.db.leaderboard.exists({ where: leaderboardData }))) { | ||
| this.logger.warn( | ||
| `session.create: leaderboard not found (userID=${userID}, mapID=${data.mapID}, gamemode=${data.gamemode}, trackType=${data.trackType}, trackNum=${data.trackNum})` | ||
| ); | ||
| return { | ||
| event: 'session.create', | ||
| data: { error: 'Leaderboard does not exist' } | ||
| }; | ||
| } | ||
|
|
||
| const sessionKey = idKey(userID); | ||
| const sessionIDs = await this.valkey.lrange(sessionKey, 0, -1); | ||
| for (const sessionID of sessionIDs) { | ||
| const session = await this.valkey.hgetall(dataKey(sessionID)); | ||
| if ( | ||
| session && | ||
| Number(session.userID) === userID && | ||
| Number(session.trackType) === data.trackType && | ||
| Number(session.trackNum) === data.trackNum | ||
| ) { | ||
| await Promise.all([ | ||
| this.valkey.lrem(sessionKey, 0, sessionID), | ||
| this.valkey.del(dataKey(sessionID)) | ||
| ]); | ||
| } | ||
| } | ||
|
|
||
| const id = await this.valkey.incr(COUNTER_KEY); | ||
| const createdAt = Date.now(); | ||
| const createdAtDate = new Date(createdAt); | ||
|
|
||
| await Promise.all([ | ||
| this.valkey.lpush(sessionKey, id), | ||
| this.valkey.hset(dataKey(id), { userID, createdAt, ...leaderboardData }), | ||
| this.valkey.lpush( | ||
| timestampKey(id), | ||
| serializeTimestamp(1, 1, 0, createdAt) | ||
| ) | ||
| ]); | ||
|
|
||
| this.logger.log( | ||
| `session.create: created session (sessionID=${id}, userID=${userID}, mapID=${data.mapID})` | ||
| ); | ||
| return { | ||
| event: 'session.create', | ||
| data: { | ||
| id, | ||
| userID, | ||
| createdAt: createdAtDate, | ||
| ...leaderboardData, | ||
| timestamps: [ | ||
| { majorNum: 1, minorNum: 1, time: 0, createdAt: createdAtDate } | ||
| ] | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
The bulk of these methods should be in a separate .service.ts file. Gateway files are effectively the same as controllers, they define the method names and DTOs for body and return time, DB/Valkey etc should be service logic.
| data: { | ||
| mapID: number; | ||
| gamemode: number; | ||
| trackType: number; | ||
| trackNum: number; | ||
| } |
There was a problem hiding this comment.
If we have class-validator setup for ws gateways, these can be annotated with DTOs like controllers do, and that'll perform validation. Otherwise people could set any garbage data and make us error.
| trackType: number; | ||
| trackNum: number; | ||
| } | ||
| ): Promise<WsResponse<unknown>> { |
There was a problem hiding this comment.
This should be typed, ideally with a DTO, like controllers do.
There was a problem hiding this comment.
Actually okay, from discussion we're agreeing to avoid returning DTOs and just use JSON.stringify, but let's still write an explicit type for these.
| "@nestjs/schedule": "6.1.2", | ||
| "@nestjs/swagger": "11.2.0", | ||
| "@nestjs/throttler": "6.5.0", | ||
| "@nestjs/websockets": "^11.1.24", |
Keep /run/session/end since it is needed for replay uploads
dcc87e7 to
9c97df7
Compare
| // These handle the live `runsession.*` messages from the game client. They | ||
| // return the response payload (a success DTO, an error DTO, or null for a | ||
| // bare ack); the GameConnectionGateway frames them into `{ event, data }`. |
There was a problem hiding this comment.
This just seems like LLM-babble to me. The message name isn't at all relevant to the service, that's for the controller. Then you're basically just summarising what the types already tell you.
Often when you ask them to some a specific change it'll insert comments like these sort of justifying the change, and it really isn't helpful.
| /** | ||
| * `runsession.create`: start a new session for the user on a leaderboard, | ||
| * clearing any existing session for the same track first. | ||
| */ |
There was a problem hiding this comment.
As above, message name isn't relevant here. I guess the rest of the comment is okay. I know I'm nitpicking here, but I'm wary of filling the repo with LLM-generated stuff.
| } | ||
| ): Promise<WsResponse<unknown>> { | ||
| @MessageBody() data: CreateRunSessionDto | ||
| ): Promise<WsResponse<RunSessionResponseDto | RunSessionErrorDto>> { |
There was a problem hiding this comment.
When we spoke about this in voice the other day I agreed that we could skip using DTOs for responses here and just use plain types. But, if we're going to use DTOs, create a wrapper DTO like this:
export class WsResponseDto<T extends Record<string, unknown>> {
@IsString()
readonly event: string;
readonly data: T | { error: string };
constructor(c: { new (): T }, event: string, data: T | { error: string }) {
this.event = event;
this.data = 'error' in data ? data : DtoFactory(c, data);
}
}That way class-transformer will apply any transformations to T, also it actually erases types so you don't need to call .toISOString explicitly (Dates stringify to ISO strings anyway).
Writing this out made me realise that we should probably be using Nest's exception filters (https://docs.nestjs.com/websockets/exception-filters) since they let you throw an unhandled error to return an error response (esp helpful for e.g. unhandled Prisma errors), and sending stuff to Sentry. But let's leave that for now, I'll probably implement in the future.
9c97df7 to
5cc9e47
Compare
2 participants
Checks
nx run db:create-migration <name>and committed the migration if I've made DB schema changesfeat: Add foo,chore: Update bar, etc...fixuped into my original commits