Skip to content

Commit

Permalink
space: Fix clobbering presence data
Browse files Browse the repository at this point in the history
Before this commit, space.updateProfileData would call presence.update
with just the updated profile data, removing the previous and current
locations that might be set in the presence data.

This commit fixes the issue by introducing an updateSelf function which
allows callers to mutate the member retrieved from getSelf and trigger a
presence update which maintains unrelated fields within the resulting
presence data.

Signed-off-by: Lewis Marshall <lewis.marshall@ably.com>
  • Loading branch information
lmars committed Aug 2, 2023
1 parent 3d4c7bb commit 96d3409
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 7 deletions.
17 changes: 17 additions & 0 deletions src/Locations.mockClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,23 @@ describe('Locations (mockClient)', () => {
await space.enter();
space.locations.set('location1');
expect(spy).toHaveBeenCalledOnce();
expect(spy).toHaveBeenCalledWith({
profileData: {},
currentLocation: 'location1',
});
});

it<SpaceTestContext>('sets previousLocation after setting location multiple times', async ({ space, presence }) => {
const spy = vi.spyOn(presence, 'update');
await space.enter();
space.locations.set('location1');
space.locations.set('location2');
expect(spy).toHaveBeenCalledTimes(2);
expect(spy).toHaveBeenNthCalledWith(2, {
profileData: {},
previousLocation: 'location1',
currentLocation: 'location2',
});
});

it<SpaceTestContext>('fires an event when a location is set', async ({ space }) => {
Expand Down
9 changes: 4 additions & 5 deletions src/Locations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,10 @@ export default class Locations extends EventEmitter<LocationEventMap> {
throw new Error('Must enter a space before setting a location');
}

return this.channel.presence.update({
profileData: self.profileData,
previousLocation: self.location,
currentLocation: location,
});
self.previousLocation = self.location;
self.location = location;

return this.space.updateSelf(self);
}

subscribe<K extends EventKey<LocationEventMap>>(
Expand Down
28 changes: 28 additions & 0 deletions src/Space.mockClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,34 @@ describe('Space (mockClient)', () => {
await space.updateProfileData((profileData) => ({ ...profileData, a: 2 }));
expect(updateSpy).toHaveBeenNthCalledWith(1, { profileData: { a: 2 } });
});

it<SpaceTestContext>('maintains the members location in presence data', async ({ presence, space }) => {
const spy = vi.spyOn(presence, 'update');
vi.spyOn(space, 'getSelf').mockReturnValue({
clientId: '1',
connectionId: 'testConnectionId',
isConnected: true,
location: null,
lastEvent: {
name: 'enter',
timestamp: 1,
},
profileData: {
a: 1,
},
});
await space.locations.set('location1');
await space.updateProfileData({ a: 2 });
expect(spy).toHaveBeenCalledTimes(2);
expect(spy).toHaveBeenNthCalledWith(1, {
profileData: { a: 1 },
currentLocation: 'location1',
});
expect(spy).toHaveBeenNthCalledWith(2, {
profileData: { a: 2 },
currentLocation: 'location1',
});
});
});
});

Expand Down
30 changes: 28 additions & 2 deletions src/Space.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export type SpaceMember = {
isConnected: boolean;
profileData: { [key: string]: any };
location: any;
previousLocation?: any;
lastEvent: {
name: Types.PresenceAction;
timestamp: number;
Expand All @@ -32,6 +33,12 @@ type SpaceLeaver = {
timeoutId: ReturnType<typeof setTimeout>;
};

interface SpaceMemberData {
profileData?: any;
currentLocation?: any;
previousLocation?: any;
}

const SPACE_OPTIONS_DEFAULTS = {
offlineTimeout: 120_000,
};
Expand Down Expand Up @@ -210,11 +217,13 @@ class Space extends EventEmitter<SpaceEventsMap> {
return;
} else if (isFunction(profileDataOrUpdateFn) && self) {
const update = profileDataOrUpdateFn(self.profileData);
await this.channel.presence.update({ profileData: update });
self.profileData = update;
await this.updateSelf(self);
return;
}

await this.channel.presence.update({ profileData: profileDataOrUpdateFn });
self.profileData = profileDataOrUpdateFn;
await this.updateSelf(self);
return;
}

Expand All @@ -234,6 +243,23 @@ class Space extends EventEmitter<SpaceEventsMap> {
return;
}

updateSelf(member: SpaceMember): Promise<void> {
const data: SpaceMemberData = {};

if (member.profileData) {
data.profileData = member.profileData;
}

if (member.location) {
data.currentLocation = member.location;
}
if (member.previousLocation) {
data.previousLocation = member.previousLocation;
}

return this.channel.presence.update(data);
}

subscribe<K extends EventKey<SpaceEventsMap>>(
listenerOrEvents?: K | K[] | EventListener<SpaceEventsMap[K]>,
listener?: EventListener<SpaceEventsMap[K]>,
Expand Down

0 comments on commit 96d3409

Please sign in to comment.