Skip to content

Commit

Permalink
Better error reporting / workaround for inconsistent selector values …
Browse files Browse the repository at this point in the history
…(#1597)

Summary:
Pull Request resolved: facebookexperimental/Recoil#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
  • Loading branch information
eagle2722 committed Feb 11, 2022
1 parent 2bd4d30 commit 32f7551
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 15 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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_**

Expand Down
42 changes: 27 additions & 15 deletions packages/recoil/caches/Recoil_TreeCache.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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};
Expand All @@ -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
}
}
}

Expand Down
33 changes: 33 additions & 0 deletions packages/recoil/recoil_values/__tests__/Recoil_selector-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ let store,
selector,
asyncSelector,
resolvingAsyncSelector,
stringAtom,
flushPromisesAndTimers,
DefaultValue,
freshSnapshot;
Expand All @@ -50,6 +51,7 @@ const testRecoil = getRecoilTestFn(() => {
({
asyncSelector,
resolvingAsyncSelector,
stringAtom,
flushPromisesAndTimers,
} = require('recoil-shared/__test_utils__/Recoil_TestingUtils'));
({noWait} = require('../Recoil_WaitFor'));
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 32f7551

Please sign in to comment.