fix(local): Use shared local master with bots (#838)

* fix(local): Use shared local master with bots

This returns a shared local master from the Local transport even when
`bots` is passed in the options to `Local`. #553 changed this to avoid
out-of-date bots persisting, but in the process broke scenarios where
several clients should remain in sync on a single page (see #800). This
implements a more careful check, returning a shared master as long as
the `bots` options passed are *equal* to each other (via simple object
equality).

The reliance on object identity could still represent a confusing
gotcha, so in the future it might be better to offer a more explicit
API for getting a new clean transport. Moving the global map of master
instances *inside* the function scope of `Local`, could do this —
calling `Local` would always return a new transport layer that could
be shared around — but would potentially break current usage where
repeatedly calling `Local` would still connect to the same master.

Fix #800

Author: delucis

src/client/transport/local.test.js

@@ -10,6 +10,7 @@ import { createStore } from 'redux';
 import { LocalTransport, LocalMaster, Local, GetBotPlayer } from './local';
 import { makeMove, gameEvent } from '../../core/action-creators';
 import { CreateGameReducer } from '../../core/reducer';
+import { ProcessGameConfig } from '../../core/game';
 import { InitializeGame } from '../../core/initialize';
 import { Client } from '../client';
 import { RandomBot } from '../../ai/random-bot';
@@ -121,6 +122,33 @@ describe('GetBotPlayer', () => {
   });
 });
 
+describe('Local', () => {
+  test('transports for same game use shared master', () => {
+    const gameKey = {};
+    const game = ProcessGameConfig(gameKey);
+    const transport1 = Local()({ game, gameKey });
+    const transport2 = Local()({ game, gameKey });
+    expect(transport1.master).toBe(transport2.master);
+  });
+
+  test('transports use shared master with bots', () => {
+    const gameKey = {};
+    const game = ProcessGameConfig(gameKey);
+    const bots = {};
+    const transport1 = Local({ bots })({ game, gameKey });
+    const transport2 = Local({ bots })({ game, gameKey });
+    expect(transport1.master).toBe(transport2.master);
+  });
+
+  test('transports use different master for different bots', () => {
+    const gameKey = {};
+    const game = ProcessGameConfig(gameKey);
+    const transport1 = Local({ bots: {} })({ game, gameKey });
+    const transport2 = Local({ bots: {} })({ game, gameKey });
+    expect(transport1.master).not.toBe(transport2.master);
+  });
+});
+
 describe('LocalMaster', () => {
   const game = {};
   const master = new LocalMaster({ game });

src/client/transport/local.ts

@@ -43,7 +43,7 @@ export function GetBotPlayer(state: State, bots: Record<PlayerID, any>) {
 
 interface LocalMasterOpts {
   game: Game;
-  bots: Record<PlayerID, any>;
+  bots?: Record<PlayerID, any>;
 }
 
 /**
@@ -234,19 +234,32 @@ export class LocalTransport extends Transport {
   }
 }
 
-const localMasters = new Map();
-export function Local(opts?: Pick<LocalMasterOpts, 'bots'>) {
+/**
+ * Global map storing local master instances.
+ */
+const localMasters: Map<
+  Game,
+  { master: LocalMaster; bots: LocalMasterOpts['bots'] }
+> = new Map();
+
+/**
+ * Create a local transport.
+ */
+export function Local({ bots }: Pick<LocalMasterOpts, 'bots'> = {}) {
   return (transportOpts: TransportOpts) => {
+    const { gameKey, game } = transportOpts;
     let master: LocalMaster;
 
-    if (localMasters.has(transportOpts.gameKey) && !opts) {
-      master = localMasters.get(transportOpts.gameKey);
-    } else {
-      master = new LocalMaster({
-        game: transportOpts.game,
-        bots: opts && opts.bots,
-      });
-      localMasters.set(transportOpts.gameKey, master);
+    if (localMasters.has(gameKey)) {
+      const instance = localMasters.get(gameKey);
+      if (instance.bots === bots) {
+        master = instance.master;
+      }
+    }
+
+    if (!master) {
+      master = new LocalMaster({ game, bots });
+      localMasters.set(gameKey, { master, bots });
     }
 
     return new LocalTransport({ master, ...transportOpts });