From 8c996720c447db6bb44c6b0bb4380ed92af2862c Mon Sep 17 00:00:00 2001 From: Dominik Piatek Date: Thu, 21 Sep 2023 09:38:41 +0100 Subject: [PATCH 1/3] Fix updateProfileData not passing current profileData as an argument --- src/Space.test.ts | 22 +++++++++++++++++-- src/Space.ts | 54 ++++++++++++++++++++++++++++++++--------------- 2 files changed, 57 insertions(+), 19 deletions(-) diff --git a/src/Space.test.ts b/src/Space.test.ts index 1f89cd66..9ad84c28 100644 --- a/src/Space.test.ts +++ b/src/Space.test.ts @@ -153,10 +153,28 @@ describe('Space', () => { presenceMap, space, }) => { - presenceMap.set('1', createPresenceMessage('enter')); + presenceMap.set( + '1', + createPresenceMessage('enter', { + data: { + profileUpdate: { + id: 1, + current: { color: 'black' }, + }, + locationUpdate: { + id: null, + current: null, + previous: null, + }, + }, + }), + ); const updateSpy = vi.spyOn(presence, 'update'); await space.updateProfileData((profileData) => ({ ...profileData, name: 'Betty' })); - expect(updateSpy).toHaveBeenNthCalledWith(1, createProfileUpdate({ current: { name: 'Betty' } })); + expect(updateSpy).toHaveBeenNthCalledWith( + 1, + createProfileUpdate({ current: { name: 'Betty', color: 'black' } }), + ); }); }); diff --git a/src/Space.ts b/src/Space.ts index ccf314f9..7ec96340 100644 --- a/src/Space.ts +++ b/src/Space.ts @@ -106,6 +106,35 @@ class Space extends EventEmitter { this.emit('update', { members: await this.members.getAll() }); } + private async createProfileUpdate(update: ProfileData) { + const self = await this.members.getSelf(); + + const profileUpdate = { + id: nanoid(), + current: update, + }; + + if (!self) { + return { + profileUpdate, + locationUpdate: { + id: null, + current: null, + previous: null, + }, + }; + } else { + return { + profileUpdate, + locationUpdate: { + id: null, + current: self.location ?? null, + previous: null, + }, + }; + } + } + async enter(profileData: ProfileData = null): Promise { return new Promise((resolve) => { const presence = this.channel.presence; @@ -137,11 +166,7 @@ class Space extends EventEmitter { }); } - async updateProfileData( - profileDataOrUpdateFn: - | Record - | ((update: Record | null) => Record), - ): Promise { + async updateProfileData(profileDataOrUpdateFn: ProfileData | ((update: ProfileData) => ProfileData)): Promise { const self = await this.members.getSelf(); if (!isObject(profileDataOrUpdateFn) && !isFunction(profileDataOrUpdateFn)) { @@ -150,24 +175,19 @@ class Space extends EventEmitter { ); } - const update = { - profileUpdate: { - id: nanoid(), - current: isFunction(profileDataOrUpdateFn) ? profileDataOrUpdateFn(null) : profileDataOrUpdateFn, - }, - locationUpdate: { - id: null, - current: self?.location ?? null, - previous: null, - }, - }; - if (!self) { + const update = await this.createProfileUpdate( + isFunction(profileDataOrUpdateFn) ? profileDataOrUpdateFn(null) : profileDataOrUpdateFn, + ); await this.presenceEnter(update); return; } + const update = await this.createProfileUpdate( + isFunction(profileDataOrUpdateFn) ? profileDataOrUpdateFn(self.profileData) : profileDataOrUpdateFn, + ); const extras = this.locks.getLockExtras(self.connectionId); + return this.presenceUpdate(update, extras); } From 5ea005aa52e8f17319234b954eb0833141d9aa92 Mon Sep 17 00:00:00 2001 From: Dominik Piatek Date: Thu, 21 Sep 2023 10:25:03 +0100 Subject: [PATCH 2/3] Fix .leave resetting profileData for users --- src/Space.test.ts | 89 ++++++++++++++++++++++++++++++++++++++++++++--- src/Space.ts | 38 ++++++++++++-------- 2 files changed, 108 insertions(+), 19 deletions(-) diff --git a/src/Space.test.ts b/src/Space.test.ts index 9ad84c28..eb0c62dd 100644 --- a/src/Space.test.ts +++ b/src/Space.test.ts @@ -192,13 +192,94 @@ describe('Space', () => { }); describe('leave', () => { - it('leaves a space successfully', async ({ presence, presenceMap, space }) => { - presenceMap.set('1', createPresenceMessage('enter')); + it('leaves a space successfully and does not nullify presence data', async ({ + presence, + presenceMap, + space, + }) => { + presenceMap.set( + '1', + createPresenceMessage('enter', { + data: { + profileUpdate: { id: 1, current: { name: 'Betty' } }, + locationUpdate: { id: null, current: { slide: 1 }, previous: null }, + }, + }), + ); - await space.enter(); const spy = vi.spyOn(presence, 'leave'); await space.leave(); - expect(spy).toHaveBeenCalledOnce(); + expect(spy).toHaveBeenNthCalledWith(1, { + profileUpdate: { + id: null, + current: { name: 'Betty' }, + }, + locationUpdate: { + id: null, + current: { slide: 1 }, + previous: null, + }, + }); + }); + + it('leaves a space successfully and nullifies presence data', async ({ + presence, + presenceMap, + space, + }) => { + presenceMap.set( + '1', + createPresenceMessage('enter', { + data: { + profileUpdate: { id: 1, current: { name: 'Betty' } }, + locationUpdate: { id: null, current: { slide: 1 }, previous: null }, + }, + }), + ); + + const spy = vi.spyOn(presence, 'leave'); + await space.leave(null); + expect(spy).toHaveBeenNthCalledWith(1, { + profileUpdate: { + id: 'NanoidID', + current: null, + }, + locationUpdate: { + id: null, + current: { slide: 1 }, + previous: null, + }, + }); + }); + + it('leaves a space successfully and updates presence data', async ({ + presence, + presenceMap, + space, + }) => { + presenceMap.set( + '1', + createPresenceMessage('enter', { + data: { + profileUpdate: { id: 1, current: { name: 'Betty' } }, + locationUpdate: { id: null, current: { slide: 1 }, previous: null }, + }, + }), + ); + + const spy = vi.spyOn(presence, 'leave'); + await space.leave({ colorWhenLeft: 'blue' }); + expect(spy).toHaveBeenNthCalledWith(1, { + profileUpdate: { + id: 'NanoidID', + current: { colorWhenLeft: 'blue' }, + }, + locationUpdate: { + id: null, + current: { slide: 1 }, + previous: null, + }, + }); }); }); diff --git a/src/Space.ts b/src/Space.ts index 7ec96340..7d84399e 100644 --- a/src/Space.ts +++ b/src/Space.ts @@ -106,9 +106,7 @@ class Space extends EventEmitter { this.emit('update', { members: await this.members.getAll() }); } - private async createProfileUpdate(update: ProfileData) { - const self = await this.members.getSelf(); - + private createProfileUpdate(self, update: ProfileData) { const profileUpdate = { id: nanoid(), current: update, @@ -177,6 +175,7 @@ class Space extends EventEmitter { if (!self) { const update = await this.createProfileUpdate( + self, isFunction(profileDataOrUpdateFn) ? profileDataOrUpdateFn(null) : profileDataOrUpdateFn, ); await this.presenceEnter(update); @@ -184,6 +183,7 @@ class Space extends EventEmitter { } const update = await this.createProfileUpdate( + self, isFunction(profileDataOrUpdateFn) ? profileDataOrUpdateFn(self.profileData) : profileDataOrUpdateFn, ); const extras = this.locks.getLockExtras(self.connectionId); @@ -198,19 +198,27 @@ class Space extends EventEmitter { throw ERR_NOT_ENTERED_SPACE(); } - const update = { - profileUpdate: { - id: profileData ? nanoid() : null, - current: profileData ?? null, - }, - locationUpdate: { - id: null, - current: self?.location ?? null, - previous: null, - }, - }; + let update; - await this.presenceLeave(update); + // Use arguments so it's possible to deliberately nullify profileData on leave + if (arguments.length > 0) { + update = this.createProfileUpdate(self, profileData); + } else { + update = { + profileUpdate: { + id: null, + current: self.profileData, + }, + locationUpdate: { + id: null, + current: self.location, + previous: null, + }, + }; + } + + const extras = this.locks.getLockExtras(self.connectionId); + await this.presenceLeave(update, extras); } async getState(): Promise<{ members: SpaceMember[] }> { From 221d988d2a67711061168a4ae04a43d7e0b8fba9 Mon Sep 17 00:00:00 2001 From: Dominik Piatek Date: Thu, 21 Sep 2023 12:25:23 +0100 Subject: [PATCH 3/3] Add SpaceUpdate class This class is responsible for creating objects that will be passed to presence. It takes care of creating an update id and encapsulates the structure of the update. --- src/Locations.ts | 24 ++--------- src/Locks.ts | 23 +++-------- src/Space.ts | 89 +++++++++-------------------------------- src/SpaceUpdate.test.ts | 71 ++++++++++++++++++++++++++++++++ src/SpaceUpdate.ts | 73 +++++++++++++++++++++++++++++++++ 5 files changed, 174 insertions(+), 106 deletions(-) create mode 100644 src/SpaceUpdate.test.ts create mode 100644 src/SpaceUpdate.ts diff --git a/src/Locations.ts b/src/Locations.ts index fc42d569..f2b57106 100644 --- a/src/Locations.ts +++ b/src/Locations.ts @@ -1,5 +1,3 @@ -import { nanoid } from 'nanoid'; - import EventEmitter, { InvalidArgumentError, inspect, @@ -11,6 +9,7 @@ import type { SpaceMember } from './types.js'; import type { PresenceMember } from './utilities/types.js'; import type Space from './Space.js'; import { ERR_NOT_ENTERED_SPACE } from './Errors.js'; +import SpaceUpdate from './SpaceUpdate.js'; type LocationsEventMap = { update: { member: SpaceMember; currentLocation: unknown; previousLocation: unknown }; @@ -19,10 +18,7 @@ type LocationsEventMap = { export default class Locations extends EventEmitter { private lastLocationUpdate: Record = {}; - constructor( - private space: Space, - private presenceUpdate: (update: PresenceMember['data'], extras?: PresenceMember['extras']) => Promise, - ) { + constructor(private space: Space, private presenceUpdate: Space['presenceUpdate']) { super(); } @@ -61,20 +57,8 @@ export default class Locations extends EventEmitter { throw ERR_NOT_ENTERED_SPACE(); } - const update: PresenceMember['data'] = { - profileUpdate: { - id: null, - current: self.profileData, - }, - locationUpdate: { - id: nanoid(), - previous: self.location, - current: location, - }, - }; - - const extras = this.space.locks.getLockExtras(self.connectionId); - await this.presenceUpdate(update, extras); + const update = new SpaceUpdate({ self, extras: this.space.locks.getLockExtras(self.connectionId) }); + await this.presenceUpdate(update.updateLocation(location)); } subscribe>( diff --git a/src/Locks.ts b/src/Locks.ts index 282221a1..c0b94b6c 100644 --- a/src/Locks.ts +++ b/src/Locks.ts @@ -11,6 +11,8 @@ import EventEmitter, { type EventListener, } from './utilities/EventEmitter.js'; +import SpaceUpdate from './SpaceUpdate.js'; + export class LockAttributes extends Map { toJSON() { return Object.fromEntries(this); @@ -34,10 +36,7 @@ export default class Locks extends EventEmitter { // have requested. private locks: Map>; - constructor( - private space: Space, - private presenceUpdate: (update: PresenceMember['data'], extras?: any) => Promise, - ) { + constructor(private space: Space, private presenceUpdate: Space['presenceUpdate']) { super(); this.locks = new Map(); } @@ -267,19 +266,9 @@ export default class Locks extends EventEmitter { pendingLock.reason = ERR_LOCK_IS_LOCKED(); } - updatePresence(member: SpaceMember) { - const update: PresenceMember['data'] = { - profileUpdate: { - id: null, - current: member.profileData, - }, - locationUpdate: { - id: null, - current: member?.location ?? null, - previous: null, - }, - }; - return this.presenceUpdate(update, this.getLockExtras(member.connectionId)); + updatePresence(self: SpaceMember) { + const update = new SpaceUpdate({ self, extras: this.getLockExtras(self.connectionId) }); + return this.presenceUpdate(update.noop()); } getLock(id: string, connectionId: string): Lock | undefined { diff --git a/src/Space.ts b/src/Space.ts index 7d84399e..50dec7d2 100644 --- a/src/Space.ts +++ b/src/Space.ts @@ -1,5 +1,4 @@ import Ably, { Types } from 'ably'; -import { nanoid } from 'nanoid'; import EventEmitter, { InvalidArgumentError, @@ -11,9 +10,9 @@ import Locations from './Locations.js'; import Cursors from './Cursors.js'; import Members from './Members.js'; import Locks from './Locks.js'; +import SpaceUpdate, { type SpacePresenceData } from './SpaceUpdate.js'; import { ERR_NOT_ENTERED_SPACE } from './Errors.js'; - import { isFunction, isObject } from './utilities/is.js'; import type { SpaceOptions, SpaceMember, ProfileData } from './types.js'; @@ -63,21 +62,21 @@ class Space extends EventEmitter { this.locks = new Locks(this, this.presenceUpdate); } - private presenceUpdate = (data: PresenceMember['data'], extras?: PresenceMember['extras']) => { + private presenceUpdate = ({ data, extras }: SpacePresenceData) => { if (!extras) { return this.channel.presence.update(data); } return this.channel.presence.update(Ably.Realtime.PresenceMessage.fromValues({ data, extras })); }; - private presenceEnter = (data: PresenceMember['data'], extras?: PresenceMember['extras']) => { + private presenceEnter = ({ data, extras }: SpacePresenceData) => { if (!extras) { return this.channel.presence.enter(data); } return this.channel.presence.enter(Ably.Realtime.PresenceMessage.fromValues({ data, extras })); }; - private presenceLeave = (data: PresenceMember['data'], extras?: PresenceMember['extras']) => { + private presenceLeave = ({ data, extras }: SpacePresenceData) => { if (!extras) { return this.channel.presence.leave(data); } @@ -106,33 +105,6 @@ class Space extends EventEmitter { this.emit('update', { members: await this.members.getAll() }); } - private createProfileUpdate(self, update: ProfileData) { - const profileUpdate = { - id: nanoid(), - current: update, - }; - - if (!self) { - return { - profileUpdate, - locationUpdate: { - id: null, - current: null, - previous: null, - }, - }; - } else { - return { - profileUpdate, - locationUpdate: { - id: null, - current: self.location ?? null, - previous: null, - }, - }; - } - } - async enter(profileData: ProfileData = null): Promise { return new Promise((resolve) => { const presence = this.channel.presence; @@ -150,17 +122,8 @@ class Space extends EventEmitter { resolve(members); }); - this.presenceEnter({ - profileUpdate: { - id: nanoid(), - current: profileData, - }, - locationUpdate: { - id: null, - current: null, - previous: null, - }, - }); + const update = new SpaceUpdate({ self: null, extras: null }); + this.presenceEnter(update.updateProfileData(profileData)); }); } @@ -173,22 +136,20 @@ class Space extends EventEmitter { ); } + let update = new SpaceUpdate({ self, extras: self ? this.locks.getLockExtras(self.connectionId) : null }); + if (!self) { - const update = await this.createProfileUpdate( - self, + const data = update.updateProfileData( isFunction(profileDataOrUpdateFn) ? profileDataOrUpdateFn(null) : profileDataOrUpdateFn, ); - await this.presenceEnter(update); + await this.presenceEnter(data); return; + } else { + const data = update.updateProfileData( + isFunction(profileDataOrUpdateFn) ? profileDataOrUpdateFn(self.profileData) : profileDataOrUpdateFn, + ); + return this.presenceUpdate(data); } - - const update = await this.createProfileUpdate( - self, - isFunction(profileDataOrUpdateFn) ? profileDataOrUpdateFn(self.profileData) : profileDataOrUpdateFn, - ); - const extras = this.locks.getLockExtras(self.connectionId); - - return this.presenceUpdate(update, extras); } async leave(profileData: ProfileData = null) { @@ -198,27 +159,17 @@ class Space extends EventEmitter { throw ERR_NOT_ENTERED_SPACE(); } - let update; + const update = new SpaceUpdate({ self, extras: this.locks.getLockExtras(self.connectionId) }); + let data; // Use arguments so it's possible to deliberately nullify profileData on leave if (arguments.length > 0) { - update = this.createProfileUpdate(self, profileData); + data = update.updateProfileData(profileData); } else { - update = { - profileUpdate: { - id: null, - current: self.profileData, - }, - locationUpdate: { - id: null, - current: self.location, - previous: null, - }, - }; + data = update.noop(); } - const extras = this.locks.getLockExtras(self.connectionId); - await this.presenceLeave(update, extras); + await this.presenceLeave(data); } async getState(): Promise<{ members: SpaceMember[] }> { diff --git a/src/SpaceUpdate.test.ts b/src/SpaceUpdate.test.ts new file mode 100644 index 00000000..aa49f926 --- /dev/null +++ b/src/SpaceUpdate.test.ts @@ -0,0 +1,71 @@ +import { describe, it, vi, expect } from 'vitest'; + +import SpaceUpdate from './SpaceUpdate.js'; +import { createSpaceMember } from './utilities/test/fakes.js'; + +vi.mock('nanoid'); + +describe('SpaceUpdate', () => { + it('creates a profileUpdate', () => { + const self = createSpaceMember({ profileData: { name: 'Berry' } }); + const update = new SpaceUpdate({ self }); + expect(update.updateProfileData({ name: 'Barry' })).toEqual({ + data: { + locationUpdate: { + current: null, + id: null, + previous: null, + }, + profileUpdate: { + current: { + name: 'Barry', + }, + id: 'NanoidID', + }, + }, + extras: undefined, + }); + }); + + it('creates a locationUpdate', () => { + const self = createSpaceMember({ location: { slide: 3 }, profileData: { name: 'Berry' } }); + const update = new SpaceUpdate({ self }); + expect(update.updateLocation({ slide: 1 }, null)).toEqual({ + data: { + locationUpdate: { + current: { slide: 1 }, + id: 'NanoidID', + previous: { slide: 3 }, + }, + profileUpdate: { + current: { + name: 'Berry', + }, + id: null, + }, + }, + extras: undefined, + }); + }); + + it('creates an object with no updates to current data', () => { + const self = createSpaceMember({ location: { slide: 3 }, profileData: { name: 'Berry' } }); + const update = new SpaceUpdate({ self }); + expect(update.noop()).toEqual({ + data: { + locationUpdate: { + current: { slide: 3 }, + id: null, + previous: null, + }, + profileUpdate: { + current: { + name: 'Berry', + }, + id: null, + }, + }, + extras: undefined, + }); + }); +}); diff --git a/src/SpaceUpdate.ts b/src/SpaceUpdate.ts new file mode 100644 index 00000000..11cebaec --- /dev/null +++ b/src/SpaceUpdate.ts @@ -0,0 +1,73 @@ +import { nanoid } from 'nanoid'; +import { Types } from 'ably'; + +import type { SpaceMember, ProfileData } from './types.js'; +import type { PresenceMember } from './utilities/types.js'; + +export interface SpacePresenceData { + data: PresenceMember['data']; + extras: PresenceMember['extras']; +} + +class SpaceUpdate { + private self: SpaceMember | null; + private extras: Types.PresenceMessage['extras']; + + constructor({ self, extras }: { self: SpaceMember | null; extras?: Types.PresenceMessage['extras'] }) { + this.self = self; + this.extras = extras; + } + + private profileUpdate(id: string | null, current: ProfileData) { + return { id, current }; + } + + private profileNoChange() { + return this.profileUpdate(null, this.self ? this.self.profileData : null); + } + + private locationUpdate(id: string | null, current: SpaceMember['location'], previous: SpaceMember['location']) { + return { id, current, previous }; + } + + private locationNoChange() { + const location = this.self ? this.self.location : null; + return this.locationUpdate(null, location, null); + } + + updateProfileData(current: ProfileData): SpacePresenceData { + return { + data: { + profileUpdate: this.profileUpdate(nanoid(), current), + locationUpdate: this.locationNoChange(), + }, + extras: this.extras, + }; + } + + updateLocation(location: SpaceMember['location'], previousLocation?: SpaceMember['location']): SpacePresenceData { + return { + data: { + profileUpdate: this.profileNoChange(), + locationUpdate: this.locationUpdate( + nanoid(), + location, + previousLocation ? previousLocation : this.self?.location, + ), + }, + extras: this.extras, + }; + } + + noop(): SpacePresenceData { + return { + data: { + profileUpdate: this.profileNoChange(), + locationUpdate: this.locationNoChange(), + }, + extras: this.extras, + }; + } +} + +export default SpaceUpdate;