Refactor dispatch and getStore naming (#203)

Author: albertogasparin

…components/__tests__/integration.test.js src/__tests__/integration.test.jssrc/components/__tests__/integration.test.js renamed to src/__tests__/integration.test.js src/components/__tests__/integration.test.js renamed to src/__tests__/integration.test.js

@@ -5,10 +5,10 @@ import React, { Fragment, memo, useEffect } from 'react';
 import { render } from '@testing-library/react';
 import { act } from 'react-dom/test-utils';
 
-import { createStore, defaultRegistry } from '../../store';
-import { createContainer } from '../container';
-import { createSubscriber } from '../subscriber';
-import { createHook } from '../hook';
+import { createStore, defaultRegistry } from '../store';
+import { createContainer } from '../components/container';
+import { createSubscriber } from '../components/subscriber';
+import { createHook } from '../components/hook';
 
 const actTick = () => act(async () => await Promise.resolve());
 
@@ -471,10 +471,8 @@ describe('Integration', () => {
     expect(handlers2.onDestroy).toHaveBeenCalledTimes(1);
   });
 
-  it('should throw an error if contained store is used without container', async () => {
-    const rejectSpy = jest
-      .spyOn(Promise, 'reject')
-      .mockImplementation(() => {});
+  it('should throw an error if contained store is used without container', () => {
+    const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
     const Store1 = createStore({
       name: 'one',
       initialState: { todos: [], loading: false },
@@ -483,12 +481,10 @@ describe('Integration', () => {
     });
 
     const Subscriber = createSubscriber(Store1);
-    render(<Subscriber>{() => null}</Subscriber>);
-    await actTick();
 
-    expect(rejectSpy).toHaveBeenCalled();
-    const [error] = rejectSpy.mock.calls[0];
-    expect(error).toEqual(expect.any(Error));
-    expect(error.message).toContain('should be contained');
+    expect(() => {
+      render(<Subscriber>{() => null}</Subscriber>);
+    }).toThrow(/should be contained/);
+    errorSpy.mockRestore();
   });
 });

src/components/__tests__/container.test.js

@@ -1,7 +1,7 @@
 /* eslint-env jest */
 
 import React from 'react';
-import { render } from '@testing-library/react';
+import { render, act } from '@testing-library/react';
 
 import { StoreMock, storeStateMock } from '../../__tests__/mocks';
 import { defaultRegistry, StoreRegistry } from '../../store/registry';
@@ -269,7 +269,7 @@ describe('Container', () => {
       const children = <Subscriber>{renderPropChildren}</Subscriber>;
       render(<Container defaultCount={5}>{children}</Container>);
       const [, { increase }] = renderPropChildren.mock.calls[0];
-      increase();
+      act(() => increase());
 
       expect(actionInner).toHaveBeenCalledWith(expect.any(Object), {
         defaultCount: 5,
@@ -287,7 +287,7 @@ describe('Container', () => {
       );
       const [, { increase }] = renderPropChildren.mock.calls[0];
       rerender(<Container defaultCount={6}>{children}</Container>);
-      increase();
+      act(() => increase());
 
       expect(actionInner).toHaveBeenCalledWith(expect.any(Object), {
         defaultCount: 6,

src/components/container.js

@@ -49,16 +49,16 @@ export default class Container extends Component {
       // These fallbacks are needed only to make enzyme shallow work
       // as it does not fully support provider-less Context enzyme#1553
       globalRegistry = defaultRegistry,
-      getStore = defaultRegistry.getStore,
+      retrieveStore = defaultRegistry.getStore,
     } = this.context;
 
     this.state = {
       api: {
         globalRegistry,
-        getStore: (Store, scope) =>
+        retrieveStore: (Store, scope) =>
           this.constructor.storeType === Store
             ? this.getScopedStore(scope)
-            : getStore(Store),
+            : retrieveStore(Store),
       },
       // stored to make them available in getDerivedStateFromProps
       // as js context there is null https://github.com/facebook/react/issues/12612
@@ -103,20 +103,15 @@ export default class Container extends Component {
     const { storeType, hooks } = this.constructor;
     const { api } = this.state;
     // we explicitly pass scope as it might be changed
-    const { storeState } = api.getStore(storeType, scope);
+    const { storeState } = api.retrieveStore(storeType, scope);
 
-    const actions = bindActions(
-      storeType.actions,
-      storeState,
-      this.getContainerProps
-    );
+    const config = {
+      props: () => this.actionProps,
+      contained: (s) => storeType === s,
+    };
 
-    this.scopedHooks = bindActions(
-      hooks,
-      storeState,
-      this.getContainerProps,
-      actions
-    );
+    const actions = bindActions(storeType.actions, storeState, config);
+    this.scopedHooks = bindActions(hooks, storeState, config, actions);
 
     // make sure we also reset actionProps
     this.actionProps = null;
@@ -146,8 +141,6 @@ export default class Container extends Component {
     return restProps;
   };
 
-  getContainerProps = () => this.actionProps;
-
   getRegistry() {
     const isLocal = !this.props.scope && !this.props.isGlobal;
     return isLocal ? this.registry : this.state.api.globalRegistry;
@@ -224,15 +217,15 @@ function useRegistry(scope, isGlobal, { globalRegistry }) {
   }, [scope, isGlobal, globalRegistry]);
 }
 
-function useContainedStore(scope, registry, props, override) {
+function useContainedStore(scope, registry, props, check, override) {
   // Store contained scopes in a map, but throwing it away on scope change
   // eslint-disable-next-line react-hooks/exhaustive-deps
   const containedStores = useMemo(() => new Map(), [scope]);
 
   // Store props in a ref to avoid re-binding actions when they change and re-rendering all
   // consumers unnecessarily. The update is handled by an effect on the component instead
-  const containerProps = useRef();
-  containerProps.current = props;
+  const propsRef = useRef();
+  propsRef.current = props;
 
   const getContainedStore = useCallback(
     (Store) => {
@@ -241,13 +234,12 @@ function useContainedStore(scope, registry, props, override) {
       // so we can provide props to actions (only triggered by children)
       if (!containedStore) {
         const isExisting = registry.hasStore(Store, scope);
-        const { storeState } = registry.getStore(Store, scope, true);
-        const getProps = () => containerProps.current;
-        const actions = bindActions(Store.actions, storeState, getProps);
+        const config = { props: () => propsRef.current, contained: check };
+        const { storeState, actions } = registry.getStore(Store, scope, config);
         const handlers = bindActions(
           { ...Store.handlers, ...override?.handlers },
           storeState,
-          getProps,
+          config,
           actions
         );
         containedStore = {
@@ -263,20 +255,20 @@ function useContainedStore(scope, registry, props, override) {
       }
       return containedStore;
     },
-    [containedStores, registry, scope, override]
+    [containedStores, registry, scope, check, override]
   );
   return [containedStores, getContainedStore];
 }
 
-function useApi(check, getContainedStore, { globalRegistry, getStore }) {
-  const getStoreRef = useRef();
-  getStoreRef.current = (Store) =>
-    check(Store) ? getContainedStore(Store) : getStore(Store);
+function useApi(check, getContainedStore, { globalRegistry, retrieveStore }) {
+  const retrieveRef = useRef();
+  retrieveRef.current = (Store) =>
+    check(Store) ? getContainedStore(Store) : retrieveStore(Store);
 
   // This api is "frozen", as changing it will trigger re-render across all consumers
-  // so we link getStore dynamically and manually call notify() on scope change
+  // so we link retrieveStore dynamically and manually call notify() on scope change
   return useMemo(
-    () => ({ globalRegistry, getStore: (s) => getStoreRef.current(s) }),
+    () => ({ globalRegistry, retrieveStore: (s) => retrieveRef.current(s) }),
     [globalRegistry]
   );
 }
@@ -294,6 +286,7 @@ function createFunctionContainer({ displayName, override } = {}) {
       scope,
       registry,
       restProps,
+      check,
       override
     );
     const api = useApi(check, getContainedStore, ctx);
@@ -329,7 +322,7 @@ function createFunctionContainer({ displayName, override } = {}) {
                 !storeState.listeners().size &&
                 // ensure registry has not already created a new store with same scope
                 storeState ===
-                  registry.getStore(Store, cachedScope, true).storeState
+                  registry.getStore(Store, cachedScope, null)?.storeState
               ) {
                 handlers.onDestroy?.();
                 registry.deleteStore(Store, cachedScope);

src/components/hook.js

@@ -9,8 +9,8 @@ const DEFAULT_SELECTOR = (state) => state;
 
 export function createHook(Store, { selector } = {}) {
   return function useSweetState(propsArg) {
-    const { getStore } = useContext(Context);
-    const { storeState, actions } = getStore(Store);
+    const { retrieveStore } = useContext(Context);
+    const { storeState, actions } = retrieveStore(Store);
 
     const hasPropsArg = propsArg !== undefined;
     const propsArgRef = useRef(propsArg);
@@ -27,9 +27,9 @@ export function createHook(Store, { selector } = {}) {
     );
 
     const getSnapshot = useCallback(() => {
-      const state = getStore(Store).storeState.getState();
+      const state = retrieveStore(Store).storeState.getState();
       return stateSelector(state, propsArgRef.current);
-    }, [getStore, stateSelector]);
+    }, [retrieveStore, stateSelector]);
 
     const currentState = useSyncExternalStore(
       storeState.subscribe,

src/context.js

@@ -7,7 +7,7 @@ import { defaultRegistry } from './store';
 export const Context = React.createContext(
   {
     globalRegistry: defaultRegistry,
-    getStore: defaultRegistry.getStore,
+    retrieveStore: (Store) => defaultRegistry.getStore(Store),
   },
   () => 0
 );

src/store/__tests__/bind-actions.test.js

@@ -6,6 +6,11 @@ import defaults from '../../defaults';
 
 jest.mock('../../defaults');
 
+const createConfigArg = ({ props = {} } = {}) => ({
+  props: () => props,
+  contained: () => false,
+});
+
 describe('bindAction', () => {
   it('should bind and call providing mutator, getState and container props', () => {
     const actionInner = jest.fn();
@@ -16,7 +21,7 @@ describe('bindAction', () => {
       storeStateMock,
       action,
       'myAction',
-      () => containerProps,
+      createConfigArg({ props: containerProps }),
       actionsMock
     );
     result(1, '2', 3);
@@ -44,7 +49,12 @@ describe('bindAction', () => {
       () =>
       ({ setState }) =>
         setState();
-    const result = bindAction(storeStateMock, action, 'myAction');
+    const result = bindAction(
+      storeStateMock,
+      action,
+      'myAction',
+      createConfigArg()
+    );
     result();
 
     expect(storeStateMock.mutator.actionName).toEqual('myAction');
@@ -60,7 +70,12 @@ describe('bindAction', () => {
       () =>
       ({ dispatch }) =>
         dispatch(action2());
-    const result = bindAction(storeStateMock, action, 'myAction2');
+    const result = bindAction(
+      storeStateMock,
+      action,
+      'myAction2',
+      createConfigArg()
+    );
     result();
 
     expect(storeStateMock.mutator.actionName).toEqual('myAction2.dispatch');
@@ -69,7 +84,7 @@ describe('bindAction', () => {
 
 describe('bindActions', () => {
   it('should return all actions bound', () => {
-    const result = bindActions(actionsMock, storeStateMock);
+    const result = bindActions(actionsMock, storeStateMock, createConfigArg());
     expect(result).toEqual({
       increase: expect.any(Function),
       decrease: expect.any(Function),
@@ -83,7 +98,7 @@ describe('bindActions', () => {
       dispatch(actionsMock.decrease())
     );
     actionsMock.decrease.mockReturnValue(({ getState }) => getState());
-    const result = bindActions(actionsMock, storeStateMock);
+    const result = bindActions(actionsMock, storeStateMock, createConfigArg());
     const output = result.increase();
 
     expect(output).toEqual(state);

src/store/__tests__/registry.test.js

@@ -20,6 +20,20 @@ describe('StoreRegistry', () => {
     expect(instance.storeState.getState()).toEqual({ count: 0 });
   });
 
+  it('should only return store if no config', () => {
+    const registry = new StoreRegistry();
+    const instance = registry.getStore(StoreMock, 's1', null);
+    expect(instance).toEqual(null);
+  });
+
+  it('should error if not contained', () => {
+    const registry = new StoreRegistry();
+    const ContainedStore = { ...StoreMock, containedBy: jest.fn() };
+    expect(() => {
+      registry.getStore(ContainedStore, 's1');
+    }).toThrow(/should be contained/);
+  });
+
   it('should say if store exists already', () => {
     const registry = new StoreRegistry();
     expect(registry.hasStore(StoreMock, 's1')).toBe(false);

src/store/bind-actions.js

@@ -13,50 +13,39 @@ export const bindAction = (
   storeState,
   actionFn,
   actionKey,
-  getContainerProps = () => {},
-  boundActions = {}
+  config,
+  actions
 ) => {
-  // Setting mutator name so we can log action name for better debuggability
-  const dispatch = (thunkFn, actionName = `${actionKey}.dispatch`) =>
+  const callThunk = (instance, thunkFn, actionName) =>
     thunkFn(
       {
         setState: defaults.devtools
-          ? namedMutator(storeState, actionName)
-          : storeState.mutator,
-        getState: storeState.getState,
+          ? namedMutator(instance.storeState, actionName)
+          : instance.storeState.mutator,
+        getState: instance.storeState.getState,
         get actions() {
           if (!warnings.has(actionFn)) {
             warnings.set(
               actionFn,
               console.warn(
                 `react-sweet-state 'actions' property has been deprecated and will be removed in the next mayor. ` +
-                  `Please check action '${actionName}' of Store '${storeState.key}' and use 'dispatch' instead`
+                  `Please check action '${actionName}' of Store '${instance.storeState.key}' and use 'dispatch' instead`
               )
             );
           }
 
-          return boundActions;
+          return actions;
         },
-        dispatch,
+        dispatch: (tFn) => callThunk(instance, tFn, `${actionName}.dispatch`),
       },
-      getContainerProps()
+      config.props()
     );
-  return (...args) => dispatch(actionFn(...args), actionKey);
+  return (...args) =>
+    callThunk({ storeState, actions }, actionFn(...args), actionKey);
 };
 
-export const bindActions = (
-  actions,
-  storeState,
-  getContainerProps = () => ({}),
-  boundActions = null
-) =>
+export const bindActions = (actions, storeState, config, boundActions = null) =>
   Object.keys(actions).reduce((acc, k) => {
-    acc[k] = bindAction(
-      storeState,
-      actions[k],
-      k,
-      getContainerProps,
-      boundActions || acc
-    );
+    acc[k] = bindAction(storeState, actions[k], k, config, boundActions || acc);
     return acc;
   }, {});