82 Fix component did update (#84)

Fixes #82
davidgilbertson · Mar 10, 2020 · b7c693f · b7c693f
1 parent f942283
commit b7c693f
Show file tree

Hide file tree

Showing 5 changed files with 167 additions and 76 deletions.
diff --git a/src/collect.tsx b/src/collect.tsx
@@ -5,6 +5,10 @@ import state from './shared/state';
 import { debug } from './shared/debug';
 import { CollectorComponent, Store, WithStoreProp } from './shared/types';
 
+// As we render down into a tree of collected components, we will start/stop
+// recording
+const componentStack: CollectorComponent[] = [];
+
 const startRecordingGetsForComponent = (component: CollectorComponent) => {
   if (!state.isInBrowser) return;
 
@@ -15,6 +19,7 @@ const startRecordingGetsForComponent = (component: CollectorComponent) => {
   });
 
   state.currentComponent = component;
+  componentStack.push(state.currentComponent);
 };
 
 const stopRecordingGetsForComponent = () => {
@@ -24,7 +29,8 @@ const stopRecordingGetsForComponent = () => {
     console.groupEnd();
   });
 
-  state.currentComponent = null;
+  componentStack.pop();
+  state.currentComponent = componentStack[componentStack.length - 1] || null;
 };
 
 type RemoveStore<T> = Pick<T, Exclude<keyof T, keyof WithStoreProp>>;

diff --git a/src/proxyHandlers.ts b/src/proxyHandlers.ts
@@ -1,5 +1,5 @@
 import { getFromNextStore, updateInNextStore } from './store';
-import { debug } from './shared/debug';
+import { logGet, logSet, logDelete } from './shared/debug';
 import state from './shared/state';
 import * as utils from './shared/utils';
 import * as paths from './shared/paths';
@@ -34,46 +34,6 @@ const addListener = (propPath: PropPath) => {
   state.listeners.set(pathString, components);
 };
 
-/**
- * Is this an attempt to get something from the store outside the render cycle?
- * This might be store.tasks.push() in a click event right after doing store.tasks = []
- * In this case, we should always return from nextStore.
- * @see setStoreTwiceInOnClick.test.js
- */
-const isGettingPropOutsideOfRenderCycle = (prop: any) =>
-  !state.currentComponent &&
-  !utils.isSymbol(prop) &&
-  prop !== 'constructor' &&
-  !state.proxyIsMuted;
-
-const logSet = (target: Target, prop: any, value?: any) => {
-  debug(() => {
-    console.groupCollapsed(`SET: ${paths.extendToUserString(target, prop)}`);
-    console.info('From:', utils.getValue(target, prop));
-    console.info('To:  ', value);
-    console.groupEnd();
-  });
-};
-
-const logGet = (target: Target, prop?: any, value?: any) => {
-  debug(() => {
-    console.groupCollapsed(`GET: ${paths.extendToUserString(target, prop)}`);
-    console.info(`Component: <${state.currentComponent!._name}>`);
-    if (typeof value !== 'undefined') {
-      console.info('Value:', value);
-    }
-    console.groupEnd();
-  });
-};
-
-const logDelete = (target: Target, prop: any) => {
-  debug(() => {
-    console.groupCollapsed(`DELETE: ${paths.extendToUserString(target, prop)}`);
-    console.info('Property: ', paths.extendToUserString(target, prop));
-    console.groupEnd();
-  });
-};
-
 /**
  * We have different handlers (different traps) for object/array and map/set.
  */
@@ -214,7 +174,16 @@ export const getHandlerForObject = <T extends Target>(
         }
 
         // TODO (davidg): does 'size' need to be below this? Would I be getting the wrong size?
-        if (isGettingPropOutsideOfRenderCycle(prop)) {
+        // Is this an attempt to get something from the store outside the
+        // render cycle? This might be store.tasks.push() in a click event
+        // right after `doing store.tasks = []`
+        // In this case, we should always return from nextStore.
+        // See setStoreTwiceInOnClick.test.js
+        if (
+          !utils.isSymbol(prop) &&
+          prop !== 'constructor' &&
+          !state.proxyIsMuted
+        ) {
           return getFromNextStore(target, prop);
         }
 

diff --git a/src/shared/debug.ts b/src/shared/debug.ts
@@ -1,3 +1,8 @@
+import { CollectorComponent, Target } from './types';
+import * as paths from './paths';
+import * as utils from './utils';
+import state from './state';
+
 const DEBUG_ON = 'on';
 const DEBUG_OFF = 'off';
 
@@ -26,3 +31,42 @@ export const debugOff = () => {
 export const debug = (cb: () => void) => {
   if (DEBUG === DEBUG_ON) cb();
 };
+
+export const logGet = (target: Target, prop?: any, value?: any) => {
+  debug(() => {
+    console.groupCollapsed(`GET: ${paths.extendToUserString(target, prop)}`);
+    console.info(`Component: <${state.currentComponent!._name}>`);
+    if (typeof value !== 'undefined') {
+      console.info('Value:', value);
+    }
+    console.groupEnd();
+  });
+};
+
+export const logSet = (target: Target, prop: any, value?: any) => {
+  debug(() => {
+    console.groupCollapsed(`SET: ${paths.extendToUserString(target, prop)}`);
+    console.info('From:', utils.getValue(target, prop));
+    console.info('To:  ', value);
+    console.groupEnd();
+  });
+};
+
+export const logDelete = (target: Target, prop: any) => {
+  debug(() => {
+    console.groupCollapsed(`DELETE: ${paths.extendToUserString(target, prop)}`);
+    console.info('Property: ', paths.extendToUserString(target, prop));
+    console.groupEnd();
+  });
+};
+
+export const logUpdate = (
+  component: CollectorComponent,
+  propsUpdated: string[]
+) => {
+  debug(() => {
+    console.groupCollapsed(`UPDATE:  <${component._name}>`);
+    console.info('Changed properties:', propsUpdated);
+    console.groupEnd();
+  });
+};
diff --git a/src/updating.ts b/src/updating.ts
@@ -1,4 +1,4 @@
-import { debug } from './shared/debug';
+import { logUpdate } from './shared/debug';
 import state from './shared/state';
 import * as paths from './shared/paths';
 import * as utils from './shared/utils';
@@ -28,11 +28,7 @@ const flushUpdates = () => {
   queue.timeoutPending = false;
 
   queue.components.forEach((propsUpdated, component) => {
-    debug(() => {
-      console.groupCollapsed(`UPDATE:  <${component._name}>`);
-      console.info('Changed properties:', Array.from(propsUpdated));
-      console.groupEnd();
-    });
+    logUpdate(component, Array.from(propsUpdated));
 
     component.update();
   });
@@ -50,7 +46,6 @@ const flushUpdates = () => {
   queue.changedPaths.clear();
 
   state.proxyIsMuted = true;
-  // TODO (davidg): rename to prevStore/store
   utils.replaceObject(state.store, state.nextStore);
   state.proxyIsMuted = false;
 };

diff --git a/tests/integration/componentDidUpdate.test.tsx b/tests/integration/componentDidUpdate.test.tsx
@@ -1,48 +1,125 @@
+// eslint-disable-next-line max-classes-per-file
 import React, { Component } from 'react';
 import { render } from '@testing-library/react';
-import { collect, store, WithStoreProp } from '../../src';
+import { collect, store as globalStore, WithStoreProp } from '../../src';
 
 interface Props extends WithStoreProp {
-  fetchData: () => {};
+  reportUserChange: (comp: string, prev: number, next: number) => {};
 }
 
-class RawClassComponent extends Component<Props> {
-  componentDidUpdate(prevProps: Props) {
-    if (this.props.store.userId !== prevProps.store.userId) {
-      this.props.fetchData();
+// This test has multiple collected components to test a specific scenario
+// https://github.com/davidgilbertson/react-recollect/issues/82
+
+const LevelTwo = collect(
+  class LevelTwoRaw extends Component<Props> {
+    componentDidUpdate(prevProps: Props) {
+      this.props.reportUserChange(
+        'LevelTwo',
+        prevProps.store.userId,
+        this.props.store.userId
+      );
+    }
+
+    render() {
+      return <p>User ID: {this.props.store.userId}</p>;
     }
   }
+);
+
+const LevelOneA = collect(
+  class LevelOneARaw extends Component<Props> {
+    componentDidUpdate(prevProps: Props) {
+      this.props.reportUserChange(
+        'LevelOneA',
+        prevProps.store.userId,
+        this.props.store.userId
+      );
+    }
 
-  render() {
-    return (
-      <div>
-        <p>User ID: {this.props.store.userId}</p>
-        <button
-          onClick={() => {
-            this.props.store.userId++;
-          }}
-        >
-          Switch user
-        </button>
-      </div>
-    );
+    render() {
+      return (
+        <div>
+          <p>User ID: {this.props.store.userId}</p>
+
+          <LevelTwo {...this.props} />
+        </div>
+      );
+    }
   }
-}
+);
 
-const ClassComponent = collect(RawClassComponent);
+const LevelOneB = collect(
+  class LevelOneBRaw extends Component<Props> {
+    componentDidUpdate(prevProps: Props) {
+      this.props.reportUserChange(
+        'LevelOneB',
+        prevProps.store.userId,
+        this.props.store.userId
+      );
+    }
 
-const fetchData = jest.fn();
+    render() {
+      return <p>User ID: {this.props.store.userId}</p>;
+    }
+  }
+);
+
+const ParentComponent = collect(
+  class ParentComponentRaw extends Component<Props> {
+    componentDidUpdate(prevProps: Props) {
+      this.props.reportUserChange(
+        'ParentComponentRaw',
+        prevProps.store.userId,
+        this.props.store.userId
+      );
+    }
+
+    render() {
+      return (
+        <div>
+          <p>User ID: {this.props.store.userId}</p>
+          <button
+            onClick={() => {
+              this.props.store.userId++;
+            }}
+          >
+            Switch user
+          </button>
+
+          <LevelOneA {...this.props} />
+          <LevelOneB {...this.props} />
+        </div>
+      );
+    }
+  }
+);
+
+const reportUserChange = jest.fn();
 
 it('should handle a change in a value', () => {
-  store.userId = 1;
+  globalStore.userId = 1;
 
-  const { getByText } = render(<ClassComponent fetchData={fetchData} />);
+  const { getByText } = render(
+    <ParentComponent
+      reportUserChange={reportUserChange}
+    />
+  );
 
-  expect(fetchData).toHaveBeenCalledTimes(0);
+  expect(reportUserChange).toHaveBeenCalledTimes(0);
 
   getByText('Switch user').click();
 
-  expect(fetchData).toHaveBeenCalledTimes(1);
+  expect(reportUserChange).toHaveBeenCalledTimes(4);
+
+  expect(reportUserChange).toHaveBeenNthCalledWith(1, 'LevelTwo', 1, 2);
+  expect(reportUserChange).toHaveBeenNthCalledWith(2, 'LevelOneA', 1, 2);
+  expect(reportUserChange).toHaveBeenNthCalledWith(3, 'LevelOneB', 1, 2);
+  expect(reportUserChange).toHaveBeenNthCalledWith(
+    4,
+    'ParentComponentRaw',
+    1,
+    2
+  );
 });
 
 // it should listen for changes on props not called in the render() method.