Repository: ctrlplusb/easy-peasy (js)

Issue:
Computed props which depend on computed props that consume store state don't update as expected
See this CodeSandbox:

[![Edit Dependent Computed Prop Issue Repro](https://codesandbox.io/static/img/play-codesandbox.svg)](https://codesandbox.io/s/dependent-computed-prop-issue-repro-rcvsgj?fontsize=14&hidenavigation=1&theme=dark)

The `readonly` computed property depends on `readonlyReason`. `readonlyReason` consumes state external to `bar` from the `foo` state slice.

When "Submit" is clicked, `readonly` should become `true` based on how the computed property is written. However it remains false. The current workaround is to define an explicit state resolver to isolate `readonlyReason`. When `readonlyReason` is accessed directly by the state resolver, `readonly` returns the expected value. See `store.ts` and try toggling the computed props to see this in action.

I believe what's happening here is that [this check](https://github.com/ctrlplusb/easy-peasy/blob/11903f863d43f4b3829d88faa3c237b316c380f5/src/computed-properties.js#L33) prevents the computed prop from re-computing any time the computed prop's inputs are the same _OR_ if store state is equal to the previous store state. In this case, the inputs are equal (with no explicit state resolvers, the default state resolvers of `[(state) => state]` are used, and no local state has changed), but store state and previous store state are different, and since the prop in question depends on a computed property that depends only on external state, its not updating correctly.

I have a possible fix in mind and will test with the help of @jmyrland before opening a PR. If for some reason this cannot be fixed, we should update the documentation to cite this as a known issue.

Pull request description:
fix: computed props w/ dependencies on store state
- Changes `storeState === prevStoreState || areInputsEqual(...)` to `storeState === prevStoreState && areInputsEqual(...)` when considering whether or not to return the cached value of a computed prop. Only if both conditions are true should we blindly return the cached value. If either condition is false, we should re-calculate and find out if the value has changed. If it has not, return the cached value still (this may happen if inputs are equal and store state has changed that the computed prop does not depend on). Otherwise update cached value and trigger re-render.
- I believe this is related to the changes introduced in #764. This PR optimized computed props to reduce/remove the number of unwanted re-renders, however the optimization was maybe a bit too aggressive and missed a few edge cases where it doesn't trigger a re-render when it should.
- The new test fails prior to this code change and illustrates the issue that prompted the filing of #873.
- This PR also removes an un-used argument from the `createComputedPropertyBinder` and changes the `runOnce` variable name to `hasRunOnce` (just for a little more clarity - this always confused be a bit at first).
 
fixes #873 

Patch:
diff --git a/src/computed-properties.js b/src/computed-properties.js
index 4d0fd05..2b739a8 100644
--- a/src/computed-properties.js
+++ b/src/computed-properties.js
@@ -1,8 +1,8 @@
 import equal from 'fast-deep-equal/es6';
 import { areInputsEqual } from './lib';
 
-export function createComputedPropertyBinder(parentPath, key, def, _r) {
-  let runOnce = false;
+export function createComputedPropertyBinder(key, def, _r) {
+  let hasRunOnce = false;
   let prevInputs = [];
   let prevValue;
   let prevStoreState;
@@ -28,10 +28,11 @@ export function createComputedPropertyBinder(parentPath, key, def, _r) {
         const inputs = def.stateResolvers.map((resolver) =>
           resolver(parentState, storeState),
         );
+
         if (
-          runOnce &&
-          (storeState === prevStoreState ||
-            areInputsEqual(inputs, prevInputs) ||
+          hasRunOnce &&
+          ((storeState === prevStoreState &&
+            areInputsEqual(inputs, prevInputs)) ||
             // We don't want computed properties resolved every time an action
             // is handled by the reducer. They need to remain lazy, only being
             // computed when used by a component or getState call;
@@ -49,7 +50,7 @@ export function createComputedPropertyBinder(parentPath, key, def, _r) {
 
         prevInputs = inputs;
         prevStoreState = storeState;
-        runOnce = true;
+        hasRunOnce = true;
         return prevValue;
       },
     });
diff --git a/src/extract-data-from-model.js b/src/extract-data-from-model.js
index 724710e..065b180 100644
--- a/src/extract-data-from-model.js
+++ b/src/extract-data-from-model.js
@@ -150,7 +150,6 @@ export default function extractDataFromModel(
         } else if (value[computedSymbol]) {
           const parent = get(parentPath, _dS);
           const bindComputedProperty = createComputedPropertyBinder(
-            parentPath,
             key,
             value,
             _r,
@@ -158,7 +157,12 @@ export default function extractDataFromModel(
           bindComputedProperty(parent, _dS);
           _cP.push({ key, parentPath, bindComputedProperty });
         } else if (value[reducerSymbol]) {
-          _cR.push({ key, parentPath, reducer: value.fn, config: value.config });
+          _cR.push({
+            key,
+            parentPath,
+            reducer: value.fn,
+            config: value.config,
+          });
         } else if (value[effectOnSymbol]) {
           const def = { ...value };
 


Test patch:
diff --git a/tests/computed.test.js b/tests/computed.test.js
index 4d18267..2d4d69b 100644
--- a/tests/computed.test.js
+++ b/tests/computed.test.js
@@ -619,3 +619,56 @@ test('computed properties operate against their original store state', () => {
   expect(stateAtAPointInTime.count).toBe(1);
   expect(store.getState().count).toBe(2);
 });
+
+test('issue #873: computed properties without state resolvers update when they have dependencies that depend on store state', () => {
+  // ARRANGE
+  const store = createStore({
+    foo: {
+      status: 'Ok',
+      setStatus: action((state, payload) => {
+        state.status = payload;
+      })
+    },
+    bar: {
+      status: 'Ok',
+      setStatus: action((state, payload) => {
+        state.status = payload;
+      })
+    },
+    baz: {
+      errorMessage: computed(
+        [
+          (_, storeState) => storeState.foo.status,
+          (_, storeState) => storeState.bar.status
+        ],
+        (fooStatus, barStatus) => {
+          if (fooStatus === 'Error' || barStatus === 'Error') {
+            return 'Uh oh, error in app!';
+          }
+
+          return '';
+        }
+      ),
+      hasError: computed((state) => Boolean(state.errorMessage))
+    }
+  });
+
+  let state = store.getState();
+
+  // ASSERT
+  expect(state.foo.status).toBe('Ok');
+  expect(state.bar.status).toBe('Ok');
+  expect(state.baz.errorMessage).toBe('');
+  expect(state.baz.hasError).toBe(false);
+
+  // ACT
+  store.getActions().foo.setStatus('Error');
+
+  state = store.getState();
+
+  // ASSERT
+  expect(state.foo.status).toBe('Error');
+  expect(state.bar.status).toBe('Ok');
+  expect(state.baz.errorMessage).toBe('Uh oh, error in app!');
+  expect(state.baz.hasError).toBe(true); // would have failed before #874
+});