102 store passed to collected components (#103)

Fixes #102

README.md

@@ -107,6 +107,7 @@ Go have a play, and when you're ready for more readme, come back to read on ...
 <!-- START doctoc generated TOC please keep comment here to allow auto update -->
 <!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
 
+
 - [API](#api)
   - [`store`](#store)
   - [`collect(ReactComponent)`](#collectreactcomponent)
@@ -131,6 +132,7 @@ Go have a play, and when you're ready for more readme, come back to read on ...
   - [What sort of stuff can go in the store?](#what-sort-of-stuff-can-go-in-the-store)
     - [Map and Set limitations](#map-and-set-limitations)
   - [When will my components be re-rendered?](#when-will-my-components-be-re-rendered)
+  - [How many components should I wrap in `collect`?](#how-many-components-should-i-wrap-in-collect)
   - [Can I use this with class-based components and functional components?](#can-i-use-this-with-class-based-components-and-functional-components)
   - [Hooks?](#hooks)
   - [Will component state still work?](#will-component-state-still-work)
@@ -861,12 +863,40 @@ Short version: when they need to be, don't worry about it.
 Longer version: if a _property_ is changed (e.g.
 `store.page.title = 'Page two'`), any component that read that property when it
 last rendered will be updated. If an object, array, map or set is changed (e.g.
-`store.tasks.pop()`), then any component that read that _target_ will be
+`store.tasks.pop()`) any component that read that object/array/map/set will be
 updated.
 
 Check out [tests/integration/updating](tests/integration/updating.test.tsx) for
 the full suite of scenarios.
 
+## How many components should I wrap in `collect`?
+
+You can wrap every component in `collect` if you feel like it. As a general
+rule, the more you wrap in `collect`, the fewer unnecessary renders you'll get,
+and the less you'll have to pass props down through your component tree.
+
+There is one rule you must follow though: do not pass part of the store _into_ a
+collected component as props. Don't worry if you're not sure what that means,
+you'll get a development-time error if you try.
+[This issue explains why](https://github.com/davidgilbertson/react-recollect/issues/102).
+
+When dealing with components that are rendered as array items (e.g. `<Product>`s
+in a `<ProductList>`), you'll probably get the best performance with the
+following setup:
+
+- Wrap the parent component in `collect`.
+- Don't wrap the child component in `collect`
+- Pass the required data to the child components as props.
+- Mark the child component as pure with `memo()` or `PureComponent`.
+
+With this arrangement, when an item in the array changes (e.g. a product is
+starred), Recollect will immutably update only that item in the store, and
+trigger the parent component to update. React will skip the update on all the
+children that didn't change, so only the one child will re-render.
+
+For a working example, see the
+[Recollect demo on CodeSandbox](https://codesandbox.io/s/lxy1mz200l).
+
 ## Can I use this with class-based components and functional components?
 
 Yep and yep.

package.json

@@ -1,6 +1,6 @@
 {
   "name": "react-recollect",
-  "version": "5.1.2",
+  "version": "5.1.3",
   "description": "Simple state management for react",
   "keywords": [
     "flux",

src/collect.tsx

@@ -2,6 +2,7 @@ import React from 'react';
 import hoistNonReactStatics from 'hoist-non-react-statics';
 import { removeListenersForComponent } from './updating';
 import * as proxyManager from './proxyManager';
+import * as paths from './shared/paths';
 import state from './shared/state';
 import { debug } from './shared/debug';
 import { CollectorComponent, Store, WithStoreProp } from './shared/types';
@@ -114,6 +115,26 @@ const collect = <C extends React.ComponentType<any>>(
       this._isMounted = true;
       this._isMounting = false;
 
+      // A user shouldn't pass data from the store into a collected component.
+      // See the issue linked in the error for details.
+      if (this.props) {
+        const recollectStoreProps: string[] = [];
+
+        // Note this is only a shallow check.
+        Object.entries(this.props).forEach(([propName, propValue]) => {
+          // If this prop has a 'path', we know it's from the Recollect store
+          // This is not good!
+          if (paths.has(propValue)) recollectStoreProps.push(propName);
+        });
+
+        // We'll just report the first match to keep the message simple
+        if (recollectStoreProps.length) {
+          console.error(
+            `You are passing part of the Recollect store from one collected component to another, which can cause unpredictable behaviour.\n Either remove the collect() wrapper from <${this._name}/>, or remove the "${recollectStoreProps[0]}" prop.\n More info: https://git.io/JvMOj`
+          );
+        }
+      }
+
       // Stop recording. For first render()
       stopRecordingGetsForComponent();
       this._isRendering = false;

src/shared/paths.ts

@@ -1,4 +1,5 @@
 import { PATH, PROP_PATH_SEP } from './constants';
+import * as utils from './utils';
 import { PropPath, Target } from './types';
 
 // Joins an array that potentially contains symbols, which need an explicit
@@ -43,7 +44,7 @@ export const addProp = (target: Target, propPath: PropPath) => {
 
 export const get = (target: Target) => target[PATH] || [];
 
-export const has = (target: Target) => PATH in target;
+export const has = (target: any) => utils.isObject(target) && PATH in target;
 
 export const set = (mutableTarget: Target, propPath: PropPath) => {
   mutableTarget[PATH] = propPath;

tests/integration/setStoreTwiceInOnClick.test.tsx

@@ -39,7 +39,7 @@ const TestComponent = collect(({ store }: WithStoreProp) => (
         <button
           onClick={() => {
             store.page.status += '!';
-            store.page.status += '!';
+            store.page = { status: `${store.page.status}!` };
             store.page.status += '!';
           }}
         >

tests/integration/sharedStoreProps.test.tsx

@@ -0,0 +1,50 @@
+import React from 'react';
+import { initStore, collect, WithStoreProp } from '../..';
+import * as testUtils from '../testUtils';
+
+beforeEach(() => {
+  initStore();
+});
+
+/**
+ * Passing anything from the store into a collected component can be trouble.
+ * @see https://github.com/davidgilbertson/react-recollect/issues/102
+ */
+it('should error when sharing between collected components', () => {
+  initStore({
+    tasks: [
+      {
+        id: 1,
+        name: 'Task one',
+      },
+    ],
+  });
+
+  const ChildComponent = ({ task }: any) => <div>{task.name}</div>;
+  const ChildComponentCollected = collect(ChildComponent);
+
+  // Shouldn't matter how many levels deep the prop is passed
+  const MiddleComponent = ({ task }: any) => (
+    <div>
+      <ChildComponentCollected task={task} />
+    </div>
+  );
+
+  const ParentComponent = collect(({ store }: WithStoreProp) => (
+    <div>
+      {!!store.tasks &&
+        store.tasks.map((task) => (
+          <MiddleComponent key={task.id} task={task} />
+        ))}
+    </div>
+  ));
+
+  const errorMessage = testUtils.expectToLogError(() => {
+    testUtils.renderStrict(<ParentComponent />);
+  });
+
+  // Just a partial match...
+  expect(errorMessage).toMatch(
+    'Either remove the collect() wrapper from <ChildComponent/>, or remove the "task" prop'
+  );
+});