From 32f7551e12eadaa3cb34b78c9db6dc73050c7ce2 Mon Sep 17 00:00:00 2001
From: eagle2722 <qatopname@gmail.com>
Date: Thu, 10 Feb 2022 18:24:47 -0800
Subject: [PATCH] Better error reporting / workaround for inconsistent selector
 values (#1597)

Summary:
Pull Request resolved: https://github.com/facebookexperimental/Recoil/pull/1597

Better detect and clarify error message when selectors do not provide consistent values based on input dependency values.

In development mode this will throw an error.  In production it will log and report to console, then try to reset the selector cache and trundle on.

This workaround is not sufficient to avoid issues and the user should resolve the selector behavior.

Reviewed By: butlersrepos

Differential Revision: D34100576

fbshipit-source-id: 1effa85f19077976b4420222005828f9fdc73a73
---
 CHANGELOG.md                                  |  1 +
 packages/recoil/caches/Recoil_TreeCache.js    | 42 ++++++++++++-------
 .../__tests__/Recoil_selector-test.js         | 33 +++++++++++++++
 3 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6b56add..b9a5379 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## UPCOMING
 
 - Avoid spurious console errors from effects when calling `setSelf()` from `onSet()` handlers. (#1589)
+- Better error reporting when selectors provide inconsistent results (#1696)
 
 **_Add new changes here as they land_**
 
diff --git a/packages/recoil/caches/Recoil_TreeCache.js b/packages/recoil/caches/Recoil_TreeCache.js
index 9ae4faa..da958bb 100644
--- a/packages/recoil/caches/Recoil_TreeCache.js
+++ b/packages/recoil/caches/Recoil_TreeCache.js
@@ -20,6 +20,7 @@ import type {
   TreeCacheNode,
 } from './Recoil_TreeCacheImplementationType';
 
+const err = require('recoil-shared/util/Recoil_err');
 const nullthrows = require('recoil-shared/util/Recoil_nullthrows');
 const recoverableViolation = require('recoil-shared/util/Recoil_recoverableViolation');
 
@@ -173,10 +174,11 @@ const addLeaf = <T>(
   value: T,
   branchKey: ?mixed,
   handlers?: SetHandlers<T>,
-  onAbort: () => void,
+  onMismatchedPath: () => void,
 ): TreeCacheNode<T> => {
   let node;
 
+  // New cache route, make new nodes
   if (root == null) {
     if (route.length === 0) {
       node = {type: 'leaf', value, parent, branchKey};
@@ -194,38 +196,48 @@ const addLeaf = <T>(
 
       node.branches.set(
         nodeValue,
-        addLeaf(null, rest, node, value, nodeValue, handlers, onAbort),
+        addLeaf(null, rest, node, value, nodeValue, handlers, onMismatchedPath),
       );
     }
+
+    // Follow an existing cache route
   } else {
     node = root;
 
+    const changedPathError =
+      'Invalid cache values.  This can happen if selectors do not return ' +
+      'consistent values for the same values of their dependencies.';
+
     if (route.length) {
-      const [path, ...rest] = route;
-      const [nodeKey, nodeValue] = path;
+      const [[nodeKey, nodeValue], ...rest] = route;
 
-      if (root.type !== 'branch' || root.nodeKey !== nodeKey) {
-        recoverableViolation(
-          'Existing cache must have a branch midway through the ' +
-            'route with matching node key. Resetting cache.',
-          'recoil',
-        );
-        onAbort();
+      if (node.type !== 'branch' || node.nodeKey !== nodeKey) {
+        recoverableViolation(changedPathError + '  Resetting cache.', 'recoil');
+        onMismatchedPath();
         return node; // ignored
       }
 
-      root.branches.set(
+      node.branches.set(
         nodeValue,
         addLeaf(
-          root.branches.get(nodeValue),
+          node.branches.get(nodeValue),
           rest,
-          root,
+          node,
           value,
           nodeValue,
           handlers,
-          onAbort,
+          onMismatchedPath,
         ),
       );
+    } else {
+      if (node.type !== 'leaf' || node.branchKey !== branchKey) {
+        if (__DEV__) {
+          throw err(changedPathError);
+        }
+        recoverableViolation(changedPathError + '  Resetting cache.', 'recoil');
+        onMismatchedPath();
+        return node; // ignored
+      }
     }
   }
 
diff --git a/packages/recoil/recoil_values/__tests__/Recoil_selector-test.js b/packages/recoil/recoil_values/__tests__/Recoil_selector-test.js
index 54b2975..52240f3 100644
--- a/packages/recoil/recoil_values/__tests__/Recoil_selector-test.js
+++ b/packages/recoil/recoil_values/__tests__/Recoil_selector-test.js
@@ -28,6 +28,7 @@ let store,
   selector,
   asyncSelector,
   resolvingAsyncSelector,
+  stringAtom,
   flushPromisesAndTimers,
   DefaultValue,
   freshSnapshot;
@@ -50,6 +51,7 @@ const testRecoil = getRecoilTestFn(() => {
   ({
     asyncSelector,
     resolvingAsyncSelector,
+    stringAtom,
     flushPromisesAndTimers,
   } = require('recoil-shared/__test_utils__/Recoil_TestingUtils'));
   ({noWait} = require('../Recoil_WaitFor'));
@@ -930,6 +932,37 @@ describe('getCallback', () => {
   });
 });
 
+testRecoil('Report error with inconsistent values', () => {
+  const depA = stringAtom();
+  const depB = stringAtom();
+
+  // NOTE This is an illegal selector because it can provide different values
+  // with the same input dependency values.
+  let invalidInput = null;
+  const mySelector = selector({
+    key: 'selector report invalid change',
+    get: ({get}) => {
+      get(depA);
+      if (invalidInput) {
+        return invalidInput;
+      }
+      return get(depB);
+    },
+  });
+
+  expect(getValue(mySelector)).toBe('DEFAULT');
+
+  invalidInput = 'INVALID';
+  setValue(depB, 'SET');
+
+  const DEV = window.__DEV__;
+  window.__DEV__ = true;
+  expect(() => getValue(mySelector)).toThrow('consistent values');
+  window.__DEV__ = false;
+  expect(getValue(mySelector)).toBe('INVALID');
+  window.__DEV__ = DEV;
+});
+
 testRecoil('Selector values are frozen', async () => {
   const devStatus = window.__DEV__;
   window.__DEV__ = true;