feat: refactored, tested and simplified client state and hooks

gmus/src/actions/actions.ts

@@ -0,0 +1,36 @@
+import { MusicPlayer } from '../types/state';
+
+interface Action<T extends string = string, P = unknown> {
+  type: T;
+  payload: P;
+}
+
+export enum ActionTypeRemote {
+  StateSet = 'STATE_SET',
+  ClientConnected = 'CLIENT_CONNECTED',
+  ClientDisconnected = 'CLIENT_DISCONNECTED',
+}
+
+// Remote actions - these only come FROM the socket
+export type ActionStateSetRemote = Action<ActionTypeRemote.StateSet, MusicPlayer | null>;
+
+export type ActionClientConnected = Action<ActionTypeRemote.ClientConnected, string[]>;
+export type ActionClientDisconnected = Action<ActionTypeRemote.ClientDisconnected, string[]>;
+
+export type RemoteAction = ActionStateSetRemote | ActionClientConnected | ActionClientDisconnected;
+
+// Local actions - these are dispatched from this client
+export enum ActionTypeLocal {
+  StateSet = '@@local/STATE_SET',
+}
+
+export type ActionStateSetLocal = Action<ActionTypeLocal.StateSet, MusicPlayer>;
+
+export const stateSet = (state: MusicPlayer): ActionStateSetLocal => ({
+  type: ActionTypeLocal.StateSet,
+  payload: state,
+});
+
+export type LocalAction = ActionStateSetLocal;
+
+export type AnyAction = LocalAction | RemoteAction;

gmus/src/actions/index.ts

@@ -0,0 +1 @@
+export * from './actions';

gmus/src/components/gmus/index.tsx

@@ -1,9 +1,8 @@
-import React, { useCallback, useEffect } from 'react';
-import { ActionTypeLocal } from '../../constants/actions';
-import { useGlobalState } from '../../hooks/reducer';
+import React, { useCallback, useState } from 'react';
 
+import { stateSet } from '../../actions';
 import { useKeepalive } from '../../hooks/socket';
-import { MusicPlayer } from '../../types/state';
+import { useGlobalState } from '../../hooks/state';
 import { ClientList } from '../client-list';
 
 export type Props = {
@@ -11,54 +10,55 @@ export type Props = {
   socket: WebSocket;
 };
 
-const testSongId = 7954; // TODO
-
 export const Gmus: React.FC<Props> = ({ myClientName, socket }) => {
   useKeepalive(socket);
 
-  const [state, dispatch] = useGlobalState(socket);
+  const [{ clientList, player }, dispatch] = useGlobalState(socket);
+
+  const [tempSongId, setTempSongId] = useState<number>(0);
 
-  const playSong = React.useCallback(() => {
-    dispatch({
-      type: ActionTypeLocal.StateSet,
-      payload: {
-        songId: testSongId,
+  const playSong = useCallback((): void => {
+    if (!tempSongId) {
+      return;
+    }
+
+    dispatch(
+      stateSet({
+        songId: tempSongId,
         playTimeSeconds: 0,
         playing: true,
         currentClient: myClientName,
-      },
-    });
-  }, [myClientName]);
-
-  const pauseSong = React.useCallback(() => {
-    setLocalState((last) => {
-      const next: MusicPlayer = { ...last, playing: false };
+      }),
+    );
+  }, [dispatch, tempSongId, myClientName]);
 
-      socket.send(
-        JSON.stringify({
-          type: 'STATE_SET',
-          payload: JSON.stringify(next),
-        }),
-      );
-
-      return next;
-    });
-  }, [ws]);
+  const playPause = useCallback(() => {
+    dispatch(
+      stateSet({
+        ...player,
+        playing: !player.playing,
+      }),
+    );
+  }, [dispatch, player]);
 
   return (
     <div>
       <div>
-        <button onClick={playSong}>Play!</button>
-        <button onClick={pauseSong}>Pause!</button>
+        <button onClick={playPause}>{player.playing ? 'Pause' : 'Play'}</button>
       </div>
-      <ClientList clients={clientList} />
       <div>
-        <h6>Local State</h6>
-        <pre>{JSON.stringify(localState, null, 2)}</pre>
+        <input
+          onChange={({ target: { value } }): void => setTempSongId(Number(value))}
+          type="number"
+          min={0}
+          step={1}
+        />
+        <button onClick={playSong}>Change track</button>
       </div>
+      <ClientList clients={clientList} />
       <div>
-        <h6>Remote State</h6>
-        <pre>{JSON.stringify(remoteState, null, 2)}</pre>
+        <h6>Player State</h6>
+        <pre>{JSON.stringify(player, null, 2)}</pre>
       </div>
     </div>
   );

gmus/src/constants/actions.ts

@@ -1,25 +0,0 @@
-import { MusicPlayer } from '../types/state';
-
-interface Action<T extends string = string, P = unknown> {
-  type: T;
-  payload: P;
-}
-
-export enum ActionTypeRemote {
-  StateSet = 'STATE_SET',
-  ClientsUpdated = 'CLIENTS_UPDATED',
-}
-
-export enum ActionTypeLocal {
-  StateSet = 'LOCAL_STATE_SET',
-}
-
-export type ActionStateSetRemote = Action<ActionTypeRemote.StateSet, MusicPlayer | null>;
-export type ActionStateSetLocal = Action<ActionTypeLocal.StateSet, MusicPlayer>;
-
-export type ActionClientsUpdated = Action<ActionTypeRemote.ClientsUpdated, string[]>;
-
-export type LocalAction = ActionStateSetLocal;
-export type RemoteAction = ActionStateSetRemote | ActionClientsUpdated;
-
-export type AnyAction = LocalAction | RemoteAction;

gmus/src/effects/effects.spec.ts

@@ -0,0 +1,25 @@
+import { ActionStateSetRemote, ActionTypeLocal, ActionTypeRemote, stateSet } from '../actions';
+import { MusicPlayer } from '../types/state';
+import { globalEffects } from './effects';
+
+describe(globalEffects.name, () => {
+  describe(ActionTypeLocal.StateSet, () => {
+    it('should create a remote state set action', () => {
+      expect.assertions(1);
+
+      const localPlayer: MusicPlayer = {
+        songId: 123,
+        playing: false,
+        playTimeSeconds: 83,
+        currentClient: 'my-client',
+      };
+
+      const result = globalEffects(stateSet(localPlayer));
+
+      expect(result).toStrictEqual<ActionStateSetRemote>({
+        type: ActionTypeRemote.StateSet,
+        payload: localPlayer,
+      });
+    });
+  });
+});

gmus/src/effects/effects.ts

@@ -0,0 +1,15 @@
+import { ActionTypeLocal, ActionTypeRemote, AnyAction, RemoteAction } from '../actions';
+
+export function globalEffects(lastAction: AnyAction | null): RemoteAction | null {
+  if (!lastAction) {
+    return null;
+  }
+
+  switch (lastAction.type) {
+    case ActionTypeLocal.StateSet:
+      return { type: ActionTypeRemote.StateSet, payload: lastAction.payload };
+
+    default:
+      return null;
+  }
+}

gmus/src/effects/index.ts

@@ -0,0 +1 @@
+export * from './effects';

gmus/src/hooks/reducer.ts

@@ -1,92 +0,0 @@
-import { Dispatch, Reducer, useCallback, useEffect, useReducer } from 'react';
-import {
-  ActionType,
-  ActionTypeLocal,
-  ActionTypeRemote,
-  AnyAction,
-  RemoteAction,
-} from '../constants/actions';
-import { MusicPlayer } from '../types/state';
-
-export type GlobalState = {
-  lastAction: AnyAction | null;
-  localPlayer: MusicPlayer;
-  remotePlayer: MusicPlayer | null;
-  clientList: string[];
-};
-
-const initialState: GlobalState = {
-  lastAction: null,
-  localPlayer: {
-    songId: null,
-    playing: false,
-    playTimeSeconds: 0,
-    currentClient: '',
-  },
-  remotePlayer: null,
-  clientList: [],
-};
-
-function init(state: GlobalState): GlobalState {
-  return state;
-}
-
-function globalReducer(state: GlobalState, action: AnyAction): GlobalState {
-  switch (action.type) {
-    case ActionTypeRemote.StateSet:
-      return { ...state, remotePlayer: action.payload };
-    case ActionTypeRemote.ClientsUpdated:
-      return { ...state, clientList: action.payload };
-    default:
-      return state;
-  }
-}
-
-function globalEffects(lastAction: AnyAction | null): RemoteAction | null {
-  if (!lastAction) {
-    return null;
-  }
-
-  switch (lastAction.type) {
-    case ActionTypeLocal.StateSet:
-      return { type: ActionTypeRemote.StateSet, payload: lastAction.payload };
-    default:
-      return null;
-  }
-}
-
-export function useGlobalState(socket: WebSocket): [GlobalState, Dispatch<AnyAction>] {
-  const [state, dispatch] = useReducer<Reducer<GlobalState, AnyAction>, GlobalState>(
-    globalReducer,
-    initialState,
-    init,
-  );
-
-  const onMessage = useCallback(({ data }: { data: string }): void => {
-    try {
-      const action = JSON.parse(data) as AnyAction;
-      dispatch(action);
-    } catch (err) {
-      console.warn('Error parsing message from websocket', err.message);
-    }
-  }, []);
-
-  useEffect(() => {
-    // eslint-disable-next-line no-param-reassign
-    socket.onmessage = onMessage;
-  }, [socket, onMessage]);
-
-  useEffect(() => {
-    const remoteEffect = globalEffects(state.lastAction);
-    if (remoteEffect) {
-      socket.send(
-        JSON.stringify({
-          type: remoteEffect.type,
-          payload: JSON.stringify(remoteEffect.payload),
-        }),
-      );
-    }
-  }, [socket, state.lastAction]);
-
-  return [state, dispatch];
-}

gmus/src/hooks/socket.ts

@@ -29,7 +29,7 @@ export function useSocket(): {
       ws = new WebSocket(`${getPubsubUrl()}?client-name=${tempName}`);
 
       ws.onopen = (): void => {
-        if (!cancelled && ws.readyState === ws.OPEN) {
+        if (!cancelled && ws && ws.readyState === ws.OPEN) {
           setError(false);
           setConnecting(false);
 

gmus/src/hooks/state.spec.tsx

@@ -0,0 +1,91 @@
+import { act, fireEvent, render, RenderResult } from '@testing-library/react';
+import WS from 'jest-websocket-mock';
+import React from 'react';
+
+import { AnyAction, RemoteAction } from '../actions';
+import * as effects from '../effects/effects';
+import * as reducer from '../reducer/reducer';
+
+import { useGlobalState } from './state';
+
+describe(useGlobalState.name, () => {
+  afterEach(WS.clean);
+
+  let server: WS;
+  beforeEach(() => {
+    server = new WS('ws://my.api:1234');
+  });
+
+  const someAction = ({
+    type: 'SOME_ACTION',
+    payload: 'yes',
+  } as unknown) as AnyAction;
+
+  const otherAction = ({
+    type: 'OTHER_ACTION',
+    payload: {
+      three: Infinity,
+    },
+  } as unknown) as RemoteAction;
+
+  const TestComponent: React.FC<{ socket: WebSocket }> = ({ socket }) => {
+    const [state, dispatch] = useGlobalState(socket);
+
+    return (
+      <>
+        <div data-testid="state">{JSON.stringify(state)}</div>
+        <button onClick={(): void => dispatch(someAction)}>Dispatch!</button>
+      </>
+    );
+  };
+
+  const setup = async (): Promise<RenderResult> => {
+    const socket = new WebSocket('ws://my.api:1234');
+    await server.connected;
+    return render(<TestComponent socket={socket} />);
+  };
+
+  describe('when a message comes in from the socket', () => {
+    it('should dispatch the action to the global reducer', async () => {
+      expect.assertions(1);
+
+      jest.spyOn(reducer, 'composedGlobalReducer').mockImplementationOnce((state, action) => {
+        if (((action as unknown) as Record<string, unknown>).type === 'OTHER_ACTION') {
+          return { ...state, it: 'worked' } as reducer.GlobalState;
+        }
+        return state;
+      });
+
+      const { getByTestId } = await setup();
+
+      act(() => {
+        server.send(JSON.stringify(otherAction));
+      });
+
+      expect(JSON.parse(getByTestId('state').innerHTML)).toStrictEqual(
+        expect.objectContaining({ it: 'worked' }),
+      );
+    });
+  });
+
+  describe('when an action is dispatched locally which produces an effect', () => {
+    it('should send the effect action to the socket', async () => {
+      expect.assertions(2);
+
+      jest.spyOn(reducer, 'globalReducer').mockReturnValueOnce({
+        ...reducer.initialState,
+        lastAction: someAction,
+      });
+
+      jest.spyOn(effects, 'globalEffects').mockReturnValueOnce(otherAction);
+
+      const { getByText } = await setup();
+      act(() => {
+        fireEvent.click(getByText('Dispatch!'));
+      });
+
+      await expect(server).toReceiveMessage(JSON.stringify(otherAction));
+      expect(server).toHaveReceivedMessages([JSON.stringify(otherAction)]);
+    });
+  });
+});

gmus/src/hooks/state.ts

@@ -0,0 +1,41 @@
+import { Dispatch, Reducer, useCallback, useEffect, useReducer } from 'react';
+
+import { AnyAction } from '../actions';
+import { globalEffects } from '../effects';
+
+import { composedGlobalReducer, GlobalState, initialState } from '../reducer';
+
+function init(state: GlobalState): GlobalState {
+  return state;
+}
+
+export function useGlobalState(socket: WebSocket): [GlobalState, Dispatch<AnyAction>] {
+  const [state, dispatch] = useReducer<Reducer<GlobalState, AnyAction>, GlobalState>(
+    composedGlobalReducer,
+    initialState,
+    init,
+  );
+
+  const onMessage = useCallback(({ data }: { data: string }): void => {
+    try {
+      const action = JSON.parse(data) as AnyAction;
+      dispatch(action);
+    } catch (err) {
+      console.warn('Error parsing message from websocket', err.message);
+    }
+  }, []);
+
+  useEffect(() => {
+    // eslint-disable-next-line no-param-reassign
+    socket.onmessage = onMessage;
+  }, [socket, onMessage]);
+
+  useEffect(() => {
+    const remoteEffect = globalEffects(state.lastAction);
+    if (remoteEffect) {
+      socket.send(JSON.stringify(remoteEffect));
+    }
+  }, [socket, state.lastAction]);
+
+  return [state, dispatch];
+}

gmus/src/index.tsx

@@ -1,13 +1,13 @@
-import React from "react";
-import ReactDOM from "react-dom";
-import { Root } from "./components/root";
-import reportWebVitals from "./reportWebVitals";
+import React from 'react';
+import ReactDOM from 'react-dom';
+import { Root } from './components/root';
+import reportWebVitals from './reportWebVitals';
 
 ReactDOM.render(
   <React.StrictMode>
     <Root />
   </React.StrictMode>,
-  document.getElementById("root")
+  document.getElementById('root'),
 );
 
 // If you want to start measuring performance in your app, pass a function

gmus/src/reducer/index.ts

@@ -0,0 +1 @@
+export * from './reducer';

gmus/src/reducer/reducer.spec.ts

@@ -0,0 +1,97 @@
+import {
+  ActionClientConnected,
+  ActionClientDisconnected,
+  ActionStateSetLocal,
+  ActionStateSetRemote,
+  ActionTypeLocal,
+  ActionTypeRemote,
+} from '../actions';
+import { composedGlobalReducer, globalReducer, initialState } from './reducer';
+
+describe(globalReducer.name, () => {
+  describe(ActionTypeRemote.StateSet, () => {
+    const action: ActionStateSetRemote = {
+      type: ActionTypeRemote.StateSet,
+      payload: {
+        songId: 123,
+        playing: true,
+        playTimeSeconds: 75,
+        currentClient: 'some-client',
+      },
+    };
+
+    it('should set the player state', () => {
+      expect.assertions(1);
+      const result = globalReducer(initialState, action);
+
+      expect(result.player).toStrictEqual({
+        songId: 123,
+        playing: true,
+        playTimeSeconds: 75,
+        currentClient: 'some-client',
+      });
+    });
+  });
+
+  describe(ActionTypeLocal.StateSet, () => {
+    const action: ActionStateSetLocal = {
+      type: ActionTypeLocal.StateSet,
+      payload: {
+        songId: 123,
+        playing: true,
+        playTimeSeconds: 75,
+        currentClient: 'some-client',
+      },
+    };
+
+    it('should set the player state', () => {
+      expect.assertions(1);
+      const result = globalReducer(initialState, action);
+
+      expect(result.player).toStrictEqual({
+        songId: 123,
+        playing: true,
+        playTimeSeconds: 75,
+        currentClient: 'some-client',
+      });
+    });
+  });
+
+  const actionClientConnected: ActionClientConnected = {
+    type: ActionTypeRemote.ClientConnected,
+    payload: ['client1', 'client2'],
+  };
+
+  const actionClientDisconnected: ActionClientDisconnected = {
+    type: ActionTypeRemote.ClientDisconnected,
+    payload: ['client1'],
+  };
+
+  describe.each`
+    actionType                             | action                      | expectedClientList
+    ${ActionTypeRemote.ClientConnected}    | ${actionClientConnected}    | ${['client1', 'client2']}
+    ${ActionTypeRemote.ClientDisconnected} | ${actionClientDisconnected} | ${['client1']}
+  `('$actionType', ({ action, expectedClientList }) => {
+    it('should update the client list', () => {
+      expect.assertions(1);
+      const result = globalReducer(initialState, action);
+
+      expect(result.clientList).toStrictEqual(expectedClientList);
+    });
+  });
+});
+
+describe(composedGlobalReducer.name, () => {
+  it('should set the lastAction prop', () => {
+    expect.assertions(1);
+
+    const action: ActionStateSetRemote = {
+      type: ActionTypeRemote.StateSet,
+      payload: null,
+    };
+
+    const result = composedGlobalReducer(initialState, action);
+
+    expect(result.lastAction).toBe(action);
+  });
+});

gmus/src/reducer/reducer.ts

@@ -0,0 +1,46 @@
+import { ActionTypeLocal, ActionTypeRemote, AnyAction } from '../actions';
+import { MusicPlayer } from '../types/state';
+
+export type GlobalState = {
+  lastAction: AnyAction | null;
+  player: MusicPlayer;
+  clientList: string[];
+};
+
+const nullPlayer: MusicPlayer = {
+  songId: null,
+  playing: false,
+  playTimeSeconds: 0,
+  currentClient: '',
+};
+
+export const initialState: GlobalState = {
+  lastAction: null,
+  player: nullPlayer,
+  clientList: [],
+};
+
+export function globalReducer(state: GlobalState, action: AnyAction): GlobalState {
+  switch (action.type) {
+    case ActionTypeRemote.StateSet:
+    case ActionTypeLocal.StateSet:
+      return { ...state, player: action.payload ?? nullPlayer };
+
+    case ActionTypeRemote.ClientConnected:
+    case ActionTypeRemote.ClientDisconnected:
+      return { ...state, clientList: action.payload };
+
+    default:
+      return state;
+  }
+}
+
+export function composedGlobalReducer(state: GlobalState, action: AnyAction): GlobalState {
+  return globalReducer(
+    {
+      ...state,
+      lastAction: action,
+    },
+    action,
+  );
+}

gmus/src/setupTests.ts

@@ -3,3 +3,7 @@
 // expect(element).toHaveTextContent(/react/i)
 // learn more: https://github.com/testing-library/jest-dom
 import '@testing-library/jest-dom';
+
+beforeEach(() => {
+  jest.restoreAllMocks();
+});

music-player/pkg/server/actions.go

@@ -24,10 +24,8 @@ func broadcastAction(thisPodClients *map[string]*Client, action *Action) []error
   var errors []error
 
   for _, client := range(*thisPodClients) {
-    if client.name != *action.FromClient {
-      if err := client.conn.WriteJSON(action); err != nil {
-	errors = append(errors, err)
-      }
+    if err := client.conn.WriteJSON(action); err != nil {
+      errors = append(errors, err)
     }
   }
 

music-player/pkg/server/clients.go

@@ -74,16 +74,15 @@ func (c *Client) subscribeToMe(l *logger.Logger, rdb *redis.Client) {
     default:
       var actionFromClient Action
       if err := c.conn.ReadJSON(&actionFromClient); err != nil {
-	l.Error("Error reading message from client: %v\n", err)
+	return
+      }
 
-      } else {
-	l.Debug("[->Client] %s (%s)\n", actionFromClient.Type, c.name)
+      l.Debug("[->Client] %s (%s)\n", actionFromClient.Type, c.name)
 
-	actionFromClient.FromClient = &c.name
+      actionFromClient.FromClient = &c.name
 
-	if err := publishAction(rdb, &actionFromClient); err != nil {
-	  l.Error("Error publishing action from client: %v\n", err)
-	}
+      if err := publishAction(rdb, &actionFromClient); err != nil {
+	l.Error("Error publishing action from client: %v\n", err)
       }
     }
   }
@@ -95,12 +94,13 @@ func (c *Client) onConnect(l *logger.Logger, rdb *redis.Client) error {
     return err
   }
 
-  go c.subscribeToMe(l, rdb)
+  c.subscribeToMe(l, rdb)
 
   return nil
 }
 
 func (c *Client) onDisconnect(l *logger.Logger, rdb *redis.Client) error {
+  l.Verbose("[Client disconnected] %s\n", c.name)
   close(c.closeChan)
 
   if err := c.disposeFromNetwork(l, rdb); err != nil {

music-player/pkg/server/pubsub.go

@@ -4,12 +4,18 @@ import (
 	"encoding/json"
 	"net/http"
 
-	"github.com/felamaslen/go-music-player/pkg/config"
 	"github.com/felamaslen/go-music-player/pkg/logger"
 	"github.com/go-redis/redis/v7"
 	"github.com/gorilla/mux"
+	"github.com/gorilla/websocket"
 )
 
+var upgrader = websocket.Upgrader{
+  CheckOrigin: func(r *http.Request) bool {
+    return true
+  },
+}
+
 func handleClientSubscription(
   l *logger.Logger,
   rdb *redis.Client,
@@ -25,7 +31,7 @@ func handleClientSubscription(
       return
     }
 
-    l.Debug("got request, name=%v\n", clientName)
+    l.Verbose("[Client connected] %s\n", clientName)
 
     conn, err := upgrader.Upgrade(w, r, nil)
     if err != nil {
@@ -97,10 +103,7 @@ func subscribeToBroadcast(
   }
 }
 
-func initPubsub(l *logger.Logger, router *mux.Router) {
-  rdb := redis.NewClient(&redis.Options{ Addr: config.GetConfig().RedisUrl })
-  defer rdb.Close()
-
+func initPubsub(l *logger.Logger, rdb *redis.Client, router *mux.Router) {
   thisPodClients := make(map[string]*Client)
   go subscribeToBroadcast(l, rdb, &thisPodClients)
 

music-player/pkg/server/server.go

@@ -7,23 +7,20 @@ import (
 
 	"github.com/felamaslen/go-music-player/pkg/config"
 	"github.com/felamaslen/go-music-player/pkg/logger"
+	"github.com/go-redis/redis/v7"
 	"github.com/gorilla/mux"
-	"github.com/gorilla/websocket"
 )
 
-var upgrader = websocket.Upgrader{
-  CheckOrigin: func(r *http.Request) bool {
-    return true
-  },
-}
-
 func StartServer() {
   conf := config.GetConfig()
   l := logger.CreateLogger(conf.LogLevel)
 
+  rdb := redis.NewClient(&redis.Options{ Addr: conf.RedisUrl })
+  defer rdb.Close()
+
   router := mux.NewRouter()
 
-  initPubsub(l, router)
+  initPubsub(l, rdb, router)
 
   port := conf.Port