Allow throwing non-errors

Summary:
Currently, if a selector throws a non Promise, the loadable will go to a 'hasValue' state. Throwing a non Promise should send the loadable into a 'hasError' state.

I've also updated the flow types to widen the `contents` type for Loadable from `Error` to `mixed` when in a 'hasError' state.

Reviewed By: drarmstr, csantos42

Differential Revision: D25622241

fbshipit-source-id: 898d13d00af38aac15decd3a8372ac78bd53b665

CHANGELOG.md

@@ -6,6 +6,7 @@
     - When subscribed selectors evaluate to the same value. (#749)
     - On initial render when not using React Concurrent Mode (#820)
 - Added useGetRecoilValueInfo_UNSTABLE() hook for dev tools. (#713, #714)
+- Bug Fix: Ensuring that throwing non Error (and non Promise) objects is supported and puts the selector into a hasError state
 
 ## 0.1.2 (2020-10-30)
 

src/adt/Recoil_Loadable.js

@@ -46,8 +46,8 @@ type Accessors<T> = $ReadOnly<{
   // Convenience accessors
   valueMaybe: () => T | void,
   valueOrThrow: () => T,
-  errorMaybe: () => Error | void,
-  errorOrThrow: () => Error,
+  errorMaybe: () => mixed | void,
+  errorOrThrow: () => mixed,
   promiseMaybe: () => Promise<T> | void,
   promiseOrThrow: () => Promise<T>,
 
@@ -58,7 +58,7 @@ type Accessors<T> = $ReadOnly<{
 
 export type Loadable<+T> =
   | $ReadOnly<{state: 'hasValue', contents: T, ...Accessors<T>}>
-  | $ReadOnly<{state: 'hasError', contents: Error, ...Accessors<T>}>
+  | $ReadOnly<{state: 'hasError', contents: mixed, ...Accessors<T>}>
   | $ReadOnly<{
       state: 'loading',
       contents: LoadablePromise<T>,
@@ -100,7 +100,12 @@ const loadableAccessors = {
 
   valueOrThrow() {
     if (this.state !== 'hasValue') {
-      throw new Error(`Loadable expected value, but in "${this.state}" state`);
+      const error = new Error(
+        `Loadable expected value, but in "${this.state}" state`,
+      );
+      // V8 keeps closures alive until stack is accessed, this prevents a memory leak
+      error.stack;
+      throw error;
     }
     return this.contents;
   },
@@ -111,7 +116,12 @@ const loadableAccessors = {
 
   errorOrThrow() {
     if (this.state !== 'hasError') {
-      throw new Error(`Loadable expected error, but in "${this.state}" state`);
+      const error = new Error(
+        `Loadable expected error, but in "${this.state}" state`,
+      );
+      // V8 keeps closures alive until stack is accessed, this prevents a memory leak
+      error.stack;
+      throw error;
     }
     return this.contents;
   },
@@ -126,9 +136,12 @@ const loadableAccessors = {
 
   promiseOrThrow(): Promise<$FlowFixMe> {
     if (this.state !== 'loading') {
-      throw new Error(
+      const error = new Error(
         `Loadable expected promise, but in "${this.state}" state`,
       );
+      // V8 keeps closures alive until stack is accessed, this prevents a memory leak
+      error.stack;
+      throw error;
     }
 
     return gkx('recoil_async_selector_refactor')
@@ -176,7 +189,10 @@ const loadableAccessors = {
           }),
       );
     }
-    throw new Error('Invalid Loadable state');
+    const error = new Error('Invalid Loadable state');
+    // V8 keeps closures alive until stack is accessed, this prevents a memory leak
+    error.stack;
+    throw error;
   },
 };
 
@@ -189,7 +205,7 @@ function loadableWithValue<T>(value: T): Loadable<T> {
   });
 }
 
-function loadableWithError<T>(error: Error): Loadable<T> {
+function loadableWithError<T>(error: mixed): Loadable<T> {
   return Object.freeze({
     state: 'hasError',
     contents: error,

src/contrib/uri_persistence/__tests__/Recoil_Link-test.js

@@ -29,7 +29,9 @@ const LinkToSnapshot = ({snapshot, children}) => (
   <LinkToRecoilSnapshot
     snapshot={snapshot}
     uriFromSnapshot={({getLoadable}) =>
-      `https://test.com/test?atom="${getLoadable(myAtom).contents.toString()}`
+      `https://test.com/test?atom="${getLoadable(myAtom)
+        .valueOrThrow()
+        .toString()}`
     }>
     {children}
   </LinkToRecoilSnapshot>
@@ -39,7 +41,9 @@ const LinkToStateChange = ({stateChange, children}) => (
   <LinkToRecoilStateChange
     stateChange={stateChange}
     uriFromSnapshot={({getLoadable}) =>
-      `https://test.com/test?atom="${getLoadable(myAtom).contents.toString()}`
+      `https://test.com/test?atom="${getLoadable(myAtom)
+        .valueOrThrow()
+        .toString()}`
     }>
     {children}
   </LinkToRecoilStateChange>
@@ -52,7 +56,7 @@ test('Link - snapshot', async () => {
     <>
       <ReadsAndWritesAtom />
       <LinkToSnapshot snapshot={snapshot}>
-        LINK-{snapshot.getLoadable(myAtom).contents.toString()}
+        LINK-{snapshot.getLoadable(myAtom).valueOrThrow().toString()}
       </LinkToSnapshot>
     </>,
   );

src/hooks/__tests__/Recoil_useRecoilValueLoadable-test.js

@@ -70,9 +70,9 @@ testRecoil('useRecoilValueLoadable - loadable with error', async () => {
     promise = expect(loadable.toPromise()).rejects.toBeInstanceOf(Error);
     expect(loadable.valueMaybe()).toBe(undefined);
     expect(() => loadable.valueOrThrow()).toThrow(Error);
-    expect((loadable.errorMaybe() ?? {}).toString()).toContain('ERROR');
+    expect(String(loadable.errorMaybe() ?? {})).toContain('ERROR');
     expect(loadable.errorOrThrow()).toBeInstanceOf(Error);
-    expect(loadable.errorOrThrow().toString()).toContain('ERROR');
+    expect(String(loadable.errorOrThrow())).toContain('ERROR');
     expect(loadable.promiseMaybe()).toBe(undefined);
     expect(() => loadable.promiseOrThrow()).toThrow(Error);
     return 'VALUE';

src/recoil_values/Recoil_selector_NEW.js

@@ -567,6 +567,7 @@ function selector<T>(
   ): [Loadable<T>, DepValues] {
     const endPerfBlock = startPerfBlock(key); // TODO T63965866: use execution ID here
     let result;
+    let resultIsError = false;
     let loadable: Loadable<T>;
 
     const depValues = new Map();
@@ -630,11 +631,12 @@ function selector<T>(
           executionId,
         ).finally(endPerfBlock);
       } else {
+        resultIsError = true;
         endPerfBlock();
       }
     }
 
-    if (result instanceof Error) {
+    if (resultIsError) {
       loadable = loadableWithError(result);
     } else if (isPromise(result)) {
       loadable = loadableWithPromise<T>(result);

src/recoil_values/__flowtests__/Recoil_WaitFor-flowtest.js

@@ -8,9 +8,10 @@
  * @flow strict-local
  * @format
  */
+
 'use strict';
 
-import type {RecoilState} from 'Recoil_RecoilValue';
+import type {RecoilState} from '../../core/Recoil_RecoilValue';
 
 const {useRecoilValue} = require('../../hooks/Recoil_Hooks');
 const atom = require('../Recoil_atom');

src/recoil_values/__tests__/Recoil_errorSelector-test.js

@@ -25,8 +25,12 @@ const testRecoil = getRecoilTestFn(() => {
   store = makeStore();
 });
 
-function getError(recoilValue) {
-  return getRecoilValueAsLoadable(store, recoilValue).errorOrThrow();
+function getError(recoilValue): Error {
+  const error = getRecoilValueAsLoadable(store, recoilValue).errorOrThrow();
+  if (!(error instanceof Error)) {
+    throw new Error('Expected error to be an instance of Error');
+  }
+  return error;
 }
 
 testRecoil('errorSelector - string', () => {

src/recoil_values/__tests__/Recoil_selector-test.js

@@ -87,8 +87,12 @@ function get(recoilValue) {
   return getLoadable(recoilValue).contents;
 }
 
-function getError(recoilValue) {
-  return getRecoilValueAsLoadable(store, recoilValue).errorOrThrow();
+function getError(recoilValue): Error {
+  const error = getRecoilValueAsLoadable(store, recoilValue).errorOrThrow();
+  if (!(error instanceof Error)) {
+    throw new Error('Expected error to be instance of Error');
+  }
+  return error;
 }
 
 function set(recoilState, value) {
@@ -202,6 +206,30 @@ testRecoil('selector - catching exceptions', () => {
   expect(get(catchingSelector)).toEqual('CAUGHT');
 });
 
+testRecoil('selector - catching exception (non Error)', () => {
+  const throwingSel = selector({
+    key: '__error/non Error thrown',
+    get: () => {
+      // eslint-disable-next-line no-throw-literal
+      throw 'MY ERROR';
+    },
+  });
+
+  const catchingSelector = selector({
+    key: 'selector/catching selector',
+    get: ({get}) => {
+      try {
+        return get(throwingSel);
+      } catch (e) {
+        expect(e).toBe('MY ERROR');
+        return 'CAUGHT';
+      }
+    },
+  });
+
+  expect(get(catchingSelector)).toEqual('CAUGHT');
+});
+
 testRecoil('selector - catching loads', () => {
   const loadingSel = resolvingAsyncSelector('READY');
   expect(get(loadingSel) instanceof Promise).toBe(true);
@@ -249,6 +277,34 @@ testRecoil('useRecoilState - selector catching exceptions', () => {
   expect(c2.textContent).toEqual('"CAUGHT"');
 });
 
+testRecoil('useRecoilState - selector catching exceptions (non Errors)', () => {
+  const throwingSel = selector({
+    key: '__error/non Error thrown',
+    get: () => {
+      // eslint-disable-next-line no-throw-literal
+      throw 'MY ERROR';
+    },
+  });
+
+  const c1 = renderElements(<ReadsAtom atom={throwingSel} />);
+  expect(c1.textContent).toEqual('error');
+
+  const catchingSelector = selector({
+    key: 'useRecoilState/catching selector',
+    get: ({get}) => {
+      try {
+        return get(throwingSel);
+      } catch (e) {
+        expect(e).toBe('MY ERROR');
+        return 'CAUGHT';
+      }
+    },
+  });
+
+  const c2 = renderElements(<ReadsAtom atom={catchingSelector} />);
+  expect(c2.textContent).toEqual('"CAUGHT"');
+});
+
 testRecoil('useRecoilState - async selector', async () => {
   const resolvingSel = resolvingAsyncSelector('READY');