From e6db84ec258e4ab8b1fb23751ef49895f1f05163 Mon Sep 17 00:00:00 2001
From: maxwellhaydn <mh.carey@pm.me>
Date: Sat, 4 Jul 2020 02:46:15 -0500
Subject: [PATCH] Make `listen` and `stop` get latest state/props (#32)

Issue #31: `listen` and `stop` were being regenerated on every render
so if you called them from inside a closure, you could end up using a
stale version that referenced old values of state and props. This led
to confusing behavior when calling `listen` or `stop` from inside a
closure, e.g.

    const { listen, stop } = useSpeechRecognition();

    const handleClick = () => {
      setTimeout(stop, 1000);
      listen();
    };

    return <button onClick={handleClick}>Listen for 1 second</button>;

Clicking the button would listen indefinitely because the version of
`stop` inside the `handleClick` callback referenced the old value of
`listening`, before `listen` was called.

This commit makes it so `listen` and `stop` always reference the latest
values of state and props so they can be safely called from within
closures.
---
 src/useSpeechRecognition.js        | 39 ++++++++++++++++---
 test/useSpeechRecognition.spec.jsx | 61 ++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/src/useSpeechRecognition.js b/src/useSpeechRecognition.js
index 0eac0e2..ac3695a 100644
--- a/src/useSpeechRecognition.js
+++ b/src/useSpeechRecognition.js
@@ -1,4 +1,33 @@
-import { useRef, useEffect, useState } from 'react';
+import { useRef, useEffect, useState, useCallback } from 'react';
+
+/**
+ * Custom hook similar to useCallback, but for callbacks where the dependencies
+ * change frequently. Ensures that references to state and props inside the
+ * callback always get the latest values. Used to keep the `listen` and `stop`
+ * functions in sync with the latest values of the `listening` and `supported`
+ * state variables. See this issue for an example of why this is needed:
+ *
+ *   https://github.com/MikeyParton/react-speech-kit/issues/31
+ *
+ * Implementation taken from "How to read an often-changing value from
+ * useCallback?" in the React hooks API reference:
+ *
+ *   https://reactjs.org/docs/hooks-faq.html#how-to-read-an-often-changing-value-from-usecallback
+ */
+const useEventCallback = (fn, dependencies) => {
+  const ref = useRef(() => {
+    throw new Error('Cannot call an event handler while rendering.');
+  });
+
+  useEffect(() => {
+    ref.current = fn;
+  }, [fn, ...dependencies]);
+
+  return useCallback((args) => {
+    const fn = ref.current;
+    return fn(args);
+  }, [ref]);
+};
 
 const useSpeechRecognition = (props = {}) => {
   const { onEnd = () => {}, onResult = () => {}, onError = () => {} } = props;
@@ -23,7 +52,7 @@ const useSpeechRecognition = (props = {}) => {
     onError(event);
   };
 
-  const listen = (args = {}) => {
+  const listen = useEventCallback((args = {}) => {
     if (listening || !supported) return;
     const {
       lang = '',
@@ -46,9 +75,9 @@ const useSpeechRecognition = (props = {}) => {
     // We want it to keep going until we tell it to stop
     recognition.current.onend = () => recognition.current.start();
     recognition.current.start();
-  };
+  }, [listening, supported, recognition]);
 
-  const stop = () => {
+  const stop = useEventCallback(() => {
     if (!listening || !supported) return;
     recognition.current.onresult = () => {};
     recognition.current.onend = () => {};
@@ -56,7 +85,7 @@ const useSpeechRecognition = (props = {}) => {
     setListening(false);
     recognition.current.stop();
     onEnd();
-  };
+  }, [listening, supported, recognition, onEnd]);
 
   useEffect(() => {
     if (typeof window === 'undefined') return;
diff --git a/test/useSpeechRecognition.spec.jsx b/test/useSpeechRecognition.spec.jsx
index 3dccb3d..cc290a9 100644
--- a/test/useSpeechRecognition.spec.jsx
+++ b/test/useSpeechRecognition.spec.jsx
@@ -21,6 +21,26 @@ describe('useSpeechRecognition', () => {
     return <TestComponent {...props} />;
   };
 
+  // A test component with `listen` and `stop` in closures to test that they
+  // reference the latest value of `listening`
+  const ClosureExample = () => {
+    const props = useSpeechRecognition({
+      onResult: mockOnResult,
+      onEnd: mockOnEnd,
+      onError: mockOnError,
+    });
+
+    props.listenHandler = () => {
+      setTimeout(() => props.listen(), 1000);
+    };
+
+    props.stopHandler = () => {
+      setTimeout(() => props.stop(), 1000);
+    };
+
+    return <TestComponent {...props} />;
+  };
+
   beforeEach(() => {
     jest.clearAllTimers();
     jest.clearAllMocks();
@@ -122,6 +142,25 @@ describe('useSpeechRecognition', () => {
         expect(MockRecognition.start.mock.calls.length).toBe(1);
       });
     });
+
+    describe('when already listening, in a closure', () => {
+      it('does not call start on the window.speechSynthesis instance again', () => {
+        jest.clearAllMocks();
+        const wrapper = mount(<ClosureExample />);
+        act(() => {
+          wrapper.find(TestComponent).props().listen();
+          wrapper.find(TestComponent).props().listenHandler();
+        });
+
+        wrapper.update();
+        act(() => {
+          jest.advanceTimersByTime(1000);
+        });
+
+        wrapper.update();
+        expect(MockRecognition.start.mock.calls.length).toBe(1);
+      });
+    });
   });
 
   describe('stop()', () => {
@@ -154,5 +193,27 @@ describe('useSpeechRecognition', () => {
         expect(mockOnEnd.mock.calls.length).toBe(0);
       });
     });
+
+    describe('while listening, in a closure', () => {
+      it('calls stop on the window.speechSynthesis instance and the provided onEnd prop, then passes listening: false', () => {
+
+        const wrapper = mount(<ClosureExample />);
+        act(() => {
+          wrapper.find(TestComponent).props().listen();
+          wrapper.find(TestComponent).props().stopHandler();
+        });
+
+        wrapper.update();
+
+        act(() => {
+          jest.advanceTimersByTime(1000);
+        });
+
+        wrapper.update();
+        expect(MockRecognition.stop.mock.calls.length).toBe(1);
+        expect(mockOnEnd.mock.calls.length).toBe(1);
+        expect(wrapper.find(TestComponent).props().listening).toBe(false);
+      });
+    });
   });
 });