Repository: ctrlplusb/easy-peasy
Pull request #141 (2019-03-27 15:51:02)

Version 2.1.2


Patch:
diff --git a/package.json b/package.json
index 92e42ca..dff477f 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "easy-peasy",
-  "version": "2.1.1",
+  "version": "2.1.2",
   "description": "Easy peasy global state for React",
   "license": "MIT",
   "main": "dist/easy-peasy.cjs.js",
diff --git a/src/create-store.js b/src/create-store.js
index e259568..3a420cc 100644
--- a/src/create-store.js
+++ b/src/create-store.js
@@ -37,7 +37,9 @@ export default function createStore(model, options = {}) {
 
   const mockActionsMiddleware = () => next => action => {
     if (mockActions) {
-      mockedActions.push(action);
+      if (action) {
+        mockedActions.push(action);
+      }
       return undefined;
     }
     return next(action);
@@ -72,10 +74,11 @@ export default function createStore(model, options = {}) {
   };
 
   const dispatchActionStringListeners = () => next => action => {
+    const result = next(action);
     if (references.internals.thunkListenersDict[action.type]) {
       dispatchThunkListeners(action.type, action.payload);
     }
-    return next(action);
+    return result;
   };
 
   const composeEnhancers =
diff --git a/src/hooks.js b/src/hooks.js
index 26c6287..fd0ecfb 100644
--- a/src/hooks.js
+++ b/src/hooks.js
@@ -6,36 +6,74 @@ import { isStateObject } from './lib';
 export function useStore(mapState, dependencies = []) {
   const store = useContext(EasyPeasyContext);
   const [state, setState] = useState(mapState(store.getState()));
+  const [error, setError] = useState(null);
   // As our effect only fires on mount and unmount it won't have the state
   // changes visible to it, therefore we use a mutable ref to track this.
   const stateRef = useRef(state);
   // Helps avoid firing of events when unsubscribed, i.e. unmounted
   const isActive = useRef(true);
+  // Tracks when a hooked component is unmounting
+  const unmounted = useRef(false);
+  // Throwing the error inline allows error boundaries the opportunity to
+  // catch the error
+  if (error) {
+    throw error;
+  }
   useEffect(() => {
+    isActive.current = true;
     const calculateState = () => {
-      const newState = mapState(store.getState());
-      isActive.current = true;
-      if (
-        newState === stateRef.current ||
-        (isStateObject(newState) &&
-          isStateObject(stateRef.current) &&
-          shallowEqual(newState, stateRef.current))
-      ) {
-        // Do nothing
+      if (!isActive.current) {
         return;
       }
-      stateRef.current = newState;
-      if (isActive.current) {
+      try {
+        const newState = mapState(store.getState());
+        if (
+          newState === stateRef.current ||
+          (isStateObject(newState) &&
+            isStateObject(stateRef.current) &&
+            shallowEqual(newState, stateRef.current))
+        ) {
+          // Do nothing
+          return;
+        }
+        stateRef.current = newState;
         setState(stateRef.current);
+      } catch (err) {
+        // see https://github.com/reduxjs/react-redux/issues/1179
+        // There is a possibility mapState will fail as the props/state that
+        // the component has received is stale. Therefore we will afford the
+        // application a small window of opportunity, where if they unmount
+        // the component or provide it with new "valid" props (which will
+        // subsequently cause a refreshed subscription cycle) then we will not
+        // throw an error.
+        // This is by no means a robust solution. We should track the
+        // associated issue in the hope for a more dependable solution.
+
+        // Setting the listener as "inactive", this can only be changed if the
+        // incoming dependencies are different (i.e. props have changed)
+        isActive.current = false;
+
+        setTimeout(() => {
+          if (!unmounted.current && !isActive.current) {
+            setError(err);
+          }
+        }, 200); // give a window of opportunity
       }
     };
     calculateState();
     const unsubscribe = store.subscribe(calculateState);
     return () => {
-      unsubscribe();
       isActive.current = false;
+      unsubscribe();
     };
   }, dependencies);
+  // This effect will set the ref value to indicate that the component has
+  // unmounted
+  useEffect(() => {
+    return () => {
+      unmounted.current = true;
+    };
+  }, []);
   return state;
 }
 
diff --git a/wallaby.js b/wallaby.js
index 56f3cc0..21d28f5 100644
--- a/wallaby.js
+++ b/wallaby.js
@@ -7,7 +7,11 @@ const babelConfigContents = fs.readFileSync(path.join(__dirname, '.babelrc'));
 const babelConfig = JSON.parse(babelConfigContents);
 
 module.exports = wallaby => ({
-  files: ['src/**/*.js', { pattern: 'src/**/*.test.js', ignore: true }],
+  files: [
+    'src/**/*.js',
+    { pattern: 'src/**/*.test.js', ignore: true },
+    { pattern: 'src/__tests__/typescript/**/*', ignore: true },
+  ],
   tests: [
     'src/**/*.test.js',
     { pattern: 'src/__tests__/typescript/**/*', ignore: true },


Test patch:
diff --git a/src/__tests__/easy-peasy.test.js b/src/__tests__/easy-peasy.test.js
index bfb8bfe..6eebd26 100644
--- a/src/__tests__/easy-peasy.test.js
+++ b/src/__tests__/easy-peasy.test.js
@@ -540,10 +540,8 @@ describe('store', () => {
     // assert
     expect(store.getMockedActions()).toEqual([
       { type: 'API_REQUEST' },
-      undefined,
-      { type: '@action.saved', payload: { success: true } },
       { type: 'API_RESPONSE', payload: { success: true } },
-      undefined,
+      { type: '@action.saved', payload: { success: true } },
       { customMiddleware: 'operateOnAPI' },
     ]);
   });
diff --git a/src/__tests__/issues.test.js b/src/__tests__/issues.test.js
new file mode 100644
index 0000000..5ede58e
--- /dev/null
+++ b/src/__tests__/issues.test.js
@@ -0,0 +1,36 @@
+import { createStore, action, listen, thunk } from '../index';
+
+test('issue#139', done => {
+  // arrange
+  const model = {
+    part1: {
+      counter1: 0,
+      incrementCounter: action(s => {
+        s.counter1 += 1;
+      }),
+    },
+    part2: {
+      counter2: 100,
+      set: action((state, payload) => {
+        state.counter2 = payload;
+      }),
+      dependent: listen(on => {
+        on(
+          model.part1.incrementCounter,
+          thunk(async (actions, payload, { getStoreState }) => {
+            const { counter1 } = getStoreState().part1;
+
+            // assert
+            expect(counter1).toEqual(1);
+
+            done();
+          }),
+        );
+      }),
+    },
+  };
+  const store = createStore(model);
+
+  // act
+  store.dispatch.part1.incrementCounter();
+});
diff --git a/src/__tests__/use-store.test.js b/src/__tests__/use-store.test.js
new file mode 100644
index 0000000..c6a4b29
--- /dev/null
+++ b/src/__tests__/use-store.test.js
@@ -0,0 +1,240 @@
+import React from 'react';
+import { act } from 'react-dom/test-utils';
+import { render, fireEvent } from 'react-testing-library';
+
+import {
+  action,
+  createStore,
+  useStore,
+  StoreProvider,
+  useActions,
+} from '../index';
+
+class ErrorBoundary extends React.Component {
+  constructor(props) {
+    super(props);
+    this.state = { hasError: false, message: '' };
+  }
+
+  static getDerivedStateFromError(error) {
+    return { hasError: true, message: error.message };
+  }
+
+  render() {
+    const { hasError, message } = this.state;
+    const { children } = this.props;
+    if (hasError) {
+      return <div data-testid="error">{message}</div>;
+    }
+    return children;
+  }
+}
+
+describe('mapState errors', () => {
+  beforeEach(() => {
+    jest.useFakeTimers();
+  });
+
+  describe('zombie children', () => {
+    test('renders successfully within window of opportunity', () => {
+      // arrange
+      const store = createStore({
+        items: {
+          a: { name: 'A' },
+          b: { name: 'B' },
+          c: { name: 'C' },
+        },
+        deleteItem: action((state, payload) => {
+          delete state.items[payload];
+        }),
+      });
+
+      const ListItem = ({ id }) => {
+        const name = useStore(s => s.items[id].name, [id]);
+        return name;
+      };
+
+      function App() {
+        const itemIds = useStore(s => Object.keys(s.items));
+        const deleteItem = useActions(a => a.deleteItem);
+        const items = itemIds.map(id => <ListItem key={id} id={id} />);
+        return (
+          <>
+            <div data-testid="items">{items}</div>
+            <button
+              data-testid="delete"
+              type="button"
+              onClick={() => deleteItem('b')}
+            >
+              Delete B
+            </button>
+          </>
+        );
+      }
+
+      const app = (
+        <StoreProvider store={store}>
+          <ErrorBoundary>
+            <App />
+          </ErrorBoundary>
+        </StoreProvider>
+      );
+
+      // act
+      const { getByTestId } = render(app);
+
+      // assert
+      expect(getByTestId('items').innerHTML).toBe('ABC');
+
+      // act
+      act(() => {
+        fireEvent.click(getByTestId('delete'));
+      });
+
+      // assert
+      expect(getByTestId('items').innerHTML).toBe('AC');
+
+      // act
+      act(() => {
+        jest.runAllTimers();
+      });
+
+      // assert
+      expect(getByTestId('items').innerHTML).toBe('AC');
+      expect(setTimeout).toHaveBeenCalledTimes(2);
+      expect(setTimeout).toHaveBeenNthCalledWith(2, expect.any(Function), 200);
+    });
+  });
+
+  describe('state getting removed', () => {
+    let app;
+    let store;
+    let App;
+
+    beforeEach(() => {
+      store = createStore({
+        todos: {
+          1: { text: 'write some tests' },
+          2: { text: 'ensure hooks work' },
+        },
+        removeTodo: action((state, payload) => {
+          delete state.todos[payload];
+        }),
+      });
+
+      function Todo({ id }) {
+        const text = useStore(s => s.todos[id].text, [id]);
+        return <div data-testid="todo">{text}</div>;
+      }
+
+      App = ({ activeTodo = 1, displayTodo = true }) => {
+        const removeTodo = useActions(a => a.removeTodo);
+        return (
+          <>
+            {displayTodo ? (
+              <Todo id={activeTodo} />
+            ) : (
+              <div data-testid="no-todo">No todo</div>
+            )}
+            <button
+              data-testid="remove"
+              onClick={() => removeTodo(activeTodo)}
+              type="button"
+            >
+              Remove todo
+            </button>
+          </>
+        );
+      };
+
+      app = (
+        <StoreProvider store={store}>
+          <ErrorBoundary>
+            <App />
+          </ErrorBoundary>
+        </StoreProvider>
+      );
+    });
+
+    test('throws an error if the window of opportunity is exceeded', () => {
+      // arrange
+      const { getByTestId } = render(app);
+
+      // act
+      act(() => {
+        // multiple store updates should be idempotent
+        fireEvent.click(getByTestId('remove'));
+        fireEvent.click(getByTestId('remove'));
+      });
+
+      // assert
+      expect(getByTestId('todo').textContent).toEqual('write some tests');
+
+      // act
+      act(() => {
+        jest.runAllTimers();
+      });
+
+      // assert
+      expect(getByTestId('error').textContent).toEqual(
+        "Cannot read property 'text' of undefined",
+      );
+    });
+
+    test('does not throw if the component unmounts', () => {
+      // arrange
+      const { getByTestId, rerender } = render(app);
+
+      // act
+      act(() => {
+        fireEvent.click(getByTestId('remove'));
+      });
+
+      // assert
+      expect(getByTestId('todo').textContent).toEqual('write some tests');
+
+      // act
+      rerender(
+        <StoreProvider store={store}>
+          <ErrorBoundary>
+            <App displayTodo={false} />
+          </ErrorBoundary>
+        </StoreProvider>,
+      );
+      act(() => {
+        jest.runAllTimers();
+      });
+
+      // assert
+      expect(getByTestId('no-todo').textContent).toEqual('No todo');
+    });
+
+    test('does not throw if the component gets new props that map to valid state', () => {
+      // arrange
+      const { getByTestId, rerender } = render(app);
+
+      // act
+      act(() => {
+        fireEvent.click(getByTestId('remove'));
+      });
+
+      // assert
+      expect(getByTestId('todo').textContent).toEqual('write some tests');
+
+      // act
+      rerender(
+        <StoreProvider store={store}>
+          <ErrorBoundary>
+            <App activeTodo={2} />
+          </ErrorBoundary>
+        </StoreProvider>,
+      );
+      act(() => {
+        jest.runAllTimers();
+      });
+
+      // assert
+      expect(getByTestId('todo').textContent).toEqual('ensure hooks work');
+    });
+  });
+});