From c9102446eae68372f8e59cfae60c9c6183cb92ed Mon Sep 17 00:00:00 2001
From: Jess Telford <jess+github@jes.st>
Date: Thu, 2 May 2019 17:05:53 +1000
Subject: [PATCH] Generalise loading of multiple Suspense-aware components
 (#1078)

---
 .changeset/98042969/changes.json              |   9 +
 .changeset/98042969/changes.md                |   1 +
 packages/admin-ui/client/pages/Item/index.js  | 267 ++++++++----------
 packages/admin-ui/package.json                |   3 +-
 packages/field-views-loader/index.js          |  23 +-
 packages/fields/src/Controller.js             |  26 +-
 .../src/types/Content/views/Controller.js     |  11 +-
 packages/utils/package.json                   |   3 +-
 packages/utils/src/index.js                   |  32 +++
 packages/utils/tests/utils.test.js            | 124 ++++++++
 10 files changed, 324 insertions(+), 175 deletions(-)
 create mode 100644 .changeset/98042969/changes.json
 create mode 100644 .changeset/98042969/changes.md

diff --git a/.changeset/98042969/changes.json b/.changeset/98042969/changes.json
new file mode 100644
index 00000000000..b8f312ecf02
--- /dev/null
+++ b/.changeset/98042969/changes.json
@@ -0,0 +1,9 @@
+{
+  "releases": [
+    { "name": "@keystone-alpha/admin-ui", "type": "patch" },
+    { "name": "@keystone-alpha/field-views-loader", "type": "patch" },
+    { "name": "@keystone-alpha/fields", "type": "minor" },
+    { "name": "@keystone-alpha/utils", "type": "minor" }
+  ],
+  "dependents": []
+}
diff --git a/.changeset/98042969/changes.md b/.changeset/98042969/changes.md
new file mode 100644
index 00000000000..d896a49d310
--- /dev/null
+++ b/.changeset/98042969/changes.md
@@ -0,0 +1 @@
+- Add a mechanism for loading multiple Suspense-aware components in parallel
\ No newline at end of file
diff --git a/packages/admin-ui/client/pages/Item/index.js b/packages/admin-ui/client/pages/Item/index.js
index ac21d5b4768..714dab83477 100644
--- a/packages/admin-ui/client/pages/Item/index.js
+++ b/packages/admin-ui/client/pages/Item/index.js
@@ -6,14 +6,13 @@ import { Mutation, Query } from 'react-apollo';
 import { withRouter } from 'react-router-dom';
 import { withToastManager } from 'react-toast-notifications';
 import memoizeOne from 'memoize-one';
-import isPromise from 'p-is-promise';
 
 import { Container } from '@arch-ui/layout';
 import { Button } from '@arch-ui/button';
 import { AutocompleteCaptor } from '@arch-ui/input';
 import { Card } from '@arch-ui/card';
 import { gridSize } from '@arch-ui/theme';
-import { mapKeys, arrayToObject, omitBy } from '@keystone-alpha/utils';
+import { mapKeys, arrayToObject, omitBy, captureSuspensePromises } from '@keystone-alpha/utils';
 
 import CreateItemModal from '../../components/CreateItemModal';
 import DeleteItemModal from '../../components/DeleteItemModal';
@@ -186,15 +185,6 @@ const ItemDetails = withRouter(
     render() {
       const { adminPath, list, updateInProgress, itemErrors, item: savedData } = this.props;
       const { item, itemHasChanged } = this.state;
-      // we want to read all of the fields before reading the views individually
-      // note we want to _read_ before not just preload because the important thing
-      // isn't doing all the requests in parallel, that already happens
-      // what we're doing here is making sure there aren't a bunch of rerenders as
-      // each of the promises resolve
-      // this probably won't be necessary with concurrent mode/maybe just other react changes
-      // also, note that this is just an optimisation, it's not strictly necessary and it should
-      // probably be removed in the future because i'm guessing this will make performance _worse_ in concurrent mode
-      list.adminMeta.readViews(list.fields.map(({ views }) => views.Field));
 
       return (
         <Fragment>
@@ -209,41 +199,39 @@ const ItemDetails = withRouter(
           <Card css={{ marginBottom: '3em', paddingBottom: 0 }}>
             <Form>
               <AutocompleteCaptor />
-              {list.fields.map((field, i) => {
-                return (
-                  <Render key={field.path}>
-                    {() => {
-                      let [Field] = field.adminMeta.readViews([field.views.Field]);
-
-                      let onChange = useCallback(
-                        value => {
-                          this.setState(({ item: itm }) => ({
-                            item: {
-                              ...itm,
-                              [field.path]: value,
-                            },
-                            itemHasChanged: true,
-                          }));
-                        },
-                        [field]
-                      );
-                      return useMemo(
-                        () => (
-                          <Field
-                            autoFocus={!i}
-                            field={field}
-                            error={itemErrors[field.path]}
-                            value={item[field.path]}
-                            onChange={onChange}
-                            renderContext="page"
-                          />
-                        ),
-                        [i, field, itemErrors[field.path], item[field.path]]
-                      );
-                    }}
-                  </Render>
-                );
-              })}
+              {list.fields.map((field, i) => (
+                <Render key={field.path}>
+                  {() => {
+                    const [Field] = field.adminMeta.readViews([field.views.Field]);
+
+                    let onChange = useCallback(
+                      value => {
+                        this.setState(({ item: itm }) => ({
+                          item: {
+                            ...itm,
+                            [field.path]: value,
+                          },
+                          itemHasChanged: true,
+                        }));
+                      },
+                      [field]
+                    );
+                    return useMemo(
+                      () => (
+                        <Field
+                          autoFocus={!i}
+                          field={field}
+                          error={itemErrors[field.path]}
+                          value={item[field.path]}
+                          onChange={onChange}
+                          renderContext="page"
+                        />
+                      ),
+                      [i, field, itemErrors[field.path], item[field.path]]
+                    );
+                  }}
+                </Render>
+              ))}
             </Form>
             <Footer
               onSave={this.onSave}
@@ -275,121 +263,92 @@ const ItemNotFound = ({ adminPath, errorMessage, list }) => (
   </PageError>
 );
 
-const PreLoadFields = ({ children, list, loadField }) => (
-  <Suspense fallback={<PageLoading />}>
-    <Render>
-      {() => {
-        const initialisationPromises = list.fields
-          .map((item, index) => {
-            try {
-              loadField(item, index, list);
-            } catch (loadingPromiseOrError) {
-              // An actual error was thrown, so we want to bubble that up
-              if (!isPromise(loadingPromiseOrError)) {
-                throw loadingPromiseOrError;
-              }
-              // Return a Suspense promise
-              return loadingPromiseOrError;
-            }
-          })
-          .filter(Boolean);
-
-        if (initialisationPromises.length) {
-          // Trigger the suspense boundary and wait for all fields in
-          // parallel.
-          throw Promise.all(initialisationPromises);
-        }
-
-        return children();
-      }}
-    </Render>
-  </Suspense>
-);
-
 const ItemPage = ({ list, itemId, adminPath, getListByKey, toastManager }) => {
   const itemQuery = list.getItemQuery(itemId);
   return (
-    <Fragment>
+    <Suspense fallback={<PageLoading />}>
       {/* network-only because the data we mutate with is important for display
           in the UI, and may be different than what's in the cache */}
       <Query query={itemQuery} fetchPolicy="network-only" errorPolicy="all">
-        {({ loading, error, data, refetch }) => (
-          <PreLoadFields list={list} loadField={field => field.initFieldView()}>
-            {() => {
-              if (loading) return <PageLoading />;
-
-              // Only show error page if there is no data
-              // (ie; there could be partial data + partial errors)
-              if (
-                error &&
-                (!data ||
-                  !data[list.gqlNames.itemQueryName] ||
-                  !Object.keys(data[list.gqlNames.itemQueryName]).length)
-              ) {
-                return (
-                  <Fragment>
-                    <DocTitle>{list.singular} not found</DocTitle>
-                    <ItemNotFound adminPath={adminPath} errorMessage={error.message} list={list} />
-                  </Fragment>
-                );
-              }
-
-              const item = list.deserializeItemData(data[list.gqlNames.itemQueryName]);
-              const itemErrors =
-                deconstructErrorsToDataShape(error)[list.gqlNames.itemQueryName] || {};
-
-              return item ? (
-                <main>
-                  <DocTitle>
-                    {item._label_} - {list.singular}
-                  </DocTitle>
-                  <Container id="toast-boundary">
-                    <Mutation
-                      mutation={list.updateMutation}
-                      onError={updateError => {
-                        const [title, ...rest] = updateError.message.split(/\:/);
-                        const toastContent = rest.length ? (
-                          <div>
-                            <strong>{title.trim()}</strong>
-                            <div>{rest.join('').trim()}</div>
-                          </div>
-                        ) : (
-                          updateError.message
-                        );
-
-                        toastManager.add(toastContent, {
-                          appearance: 'error',
-                        });
-                      }}
-                    >
-                      {(updateItem, { loading: updateInProgress, error: updateError }) => {
-                        return (
-                          <ItemDetails
-                            adminPath={adminPath}
-                            item={item}
-                            itemErrors={itemErrors}
-                            key={itemId}
-                            list={list}
-                            getListByKey={getListByKey}
-                            onUpdate={refetch}
-                            toastManager={toastManager}
-                            updateInProgress={updateInProgress}
-                            updateErrorMessage={updateError && updateError.message}
-                            updateItem={updateItem}
-                          />
-                        );
-                      }}
-                    </Mutation>
-                  </Container>
-                </main>
-              ) : (
-                <ItemNotFound adminPath={adminPath} list={list} />
-              );
-            }}
-          </PreLoadFields>
-        )}
+        {({ loading, error, data, refetch }) => {
+          // Now that the network request for data has been triggered, we
+          // try to initialise the fields. They are Suspense capable, so may
+          // throw Promises which will be caught by the above <Suspense>
+          captureSuspensePromises(list.fields.map(field => () => field.initFieldView()));
+
+          // If the views load before the API request comes back, keep showing
+          // the loading component
+          if (loading) return <PageLoading />;
+
+          // Only show error page if there is no data
+          // (ie; there could be partial data + partial errors)
+          if (
+            error &&
+            (!data ||
+              !data[list.gqlNames.itemQueryName] ||
+              !Object.keys(data[list.gqlNames.itemQueryName]).length)
+          ) {
+            return (
+              <Fragment>
+                <DocTitle>{list.singular} not found</DocTitle>
+                <ItemNotFound adminPath={adminPath} errorMessage={error.message} list={list} />
+              </Fragment>
+            );
+          }
+
+          const item = list.deserializeItemData(data[list.gqlNames.itemQueryName]);
+          const itemErrors = deconstructErrorsToDataShape(error)[list.gqlNames.itemQueryName] || {};
+
+          return item ? (
+            <main>
+              <DocTitle>
+                {item._label_} - {list.singular}
+              </DocTitle>
+              <Container id="toast-boundary">
+                <Mutation
+                  mutation={list.updateMutation}
+                  onError={updateError => {
+                    const [title, ...rest] = updateError.message.split(/\:/);
+                    const toastContent = rest.length ? (
+                      <div>
+                        <strong>{title.trim()}</strong>
+                        <div>{rest.join('').trim()}</div>
+                      </div>
+                    ) : (
+                      updateError.message
+                    );
+
+                    toastManager.add(toastContent, {
+                      appearance: 'error',
+                    });
+                  }}
+                >
+                  {(updateItem, { loading: updateInProgress, error: updateError }) => {
+                    return (
+                      <ItemDetails
+                        adminPath={adminPath}
+                        item={item}
+                        itemErrors={itemErrors}
+                        key={itemId}
+                        list={list}
+                        getListByKey={getListByKey}
+                        onUpdate={refetch}
+                        toastManager={toastManager}
+                        updateInProgress={updateInProgress}
+                        updateErrorMessage={updateError && updateError.message}
+                        updateItem={updateItem}
+                      />
+                    );
+                  }}
+                </Mutation>
+              </Container>
+            </main>
+          ) : (
+            <ItemNotFound adminPath={adminPath} list={list} />
+          );
+        }}
       </Query>
-    </Fragment>
+    </Suspense>
   );
 };
 
diff --git a/packages/admin-ui/package.json b/packages/admin-ui/package.json
index 5c7fc4db45c..c9e206649ad 100644
--- a/packages/admin-ui/package.json
+++ b/packages/admin-ui/package.json
@@ -70,7 +70,6 @@
     "lodash.debounce": "^4.0.8",
     "lodash.set": "^4.3.2",
     "memoize-one": "^4.0.2",
-    "p-is-promise": "^2.1.0",
     "preconstruct": "0.0.60",
     "prop-types": "^15.7.2",
     "raf-schd": "^4.0.0",
@@ -93,4 +92,4 @@
     "webpack-dev-middleware": "^3.6.1",
     "webpack-hot-middleware": "^2.24.3"
   }
-}
\ No newline at end of file
+}
diff --git a/packages/field-views-loader/index.js b/packages/field-views-loader/index.js
index 73032e468d9..7d2286360c4 100644
--- a/packages/field-views-loader/index.js
+++ b/packages/field-views-loader/index.js
@@ -105,6 +105,7 @@ module.exports = function() {
     .join(',\n')}\n}`;
 
   const source = endent`
+    import { captureSuspensePromises } from '@keystone-alpha/utils';
     let promiseCache = new Map();
     let valueCache = new Map();
 
@@ -124,19 +125,15 @@ module.exports = function() {
     }
 
     export function readViews(views) {
-      let promises = [];
-      let values = [];
-      views.forEach(view => {
-        if (valueCache.has(view)) {
-          values.push(valueCache.get(view));
-        } else {
-          promises.push(loadView(view));
-        }
-      });
-      if (promises.length) {
-        throw Promise.all(promises);
-      }
-      return values;
+      return captureSuspensePromises(
+        views.map(view => () => {
+          if (valueCache.has(view)) {
+            return valueCache.get(view);
+          } else {
+            throw loadView(view);
+          }
+        })
+      );
     }
 
     function interopDefault(mod) {
diff --git a/packages/fields/src/Controller.js b/packages/fields/src/Controller.js
index f5bb2745810..700ef47a06d 100644
--- a/packages/fields/src/Controller.js
+++ b/packages/fields/src/Controller.js
@@ -46,7 +46,27 @@ export default class FieldController {
   // eslint-disable-next-line no-unused-vars
   getDefaultValue = data => this.config.defaultValue || '';
 
-  initCellView = () => {};
-  initFieldView = () => {};
-  initFilterView = () => {};
+  initCellView = () => {
+    const { Cell } = this.views;
+    if (!Cell) {
+      return;
+    }
+    this.adminMeta.readViews([Cell]);
+  };
+
+  initFieldView = () => {
+    const { Field } = this.views;
+    if (!Field) {
+      return;
+    }
+    this.adminMeta.readViews([Field]);
+  };
+
+  initFilterView = () => {
+    const { Filter } = this.views;
+    if (!Filter) {
+      return;
+    }
+    this.adminMeta.readViews([Filter]);
+  };
 }
diff --git a/packages/fields/src/types/Content/views/Controller.js b/packages/fields/src/types/Content/views/Controller.js
index 24155bd3a4a..5370b86a710 100644
--- a/packages/fields/src/types/Content/views/Controller.js
+++ b/packages/fields/src/types/Content/views/Controller.js
@@ -4,7 +4,7 @@
 // should import from '@keystone-alpha/fields/types/Text/views/Controller'
 import memoizeOne from 'memoize-one';
 import isPromise from 'p-is-promise';
-import { omitBy } from '@keystone-alpha/utils';
+import { omitBy, captureSuspensePromises } from '@keystone-alpha/utils';
 import { Value } from 'slate';
 
 import TextController from '../../Text/views/Controller';
@@ -60,6 +60,8 @@ export default class ContentController extends TextController {
 
       return flattenBlocks(customBlocks);
     });
+
+    this.initFieldView = this.initFieldView.bind(this);
   }
 
   serialize = data => {
@@ -125,5 +127,10 @@ export default class ContentController extends TextController {
     `;
   };
 
-  initFieldView = () => this.getBlocks();
+  // NOTE: To use `super` below, this must be a method which is bound to `this`
+  // within the constructor. This is due to the way Babel does magic compilation
+  // of classes.
+  initFieldView() {
+    captureSuspensePromises([() => super.initFieldView(), () => this.getBlocks()]);
+  }
 }
diff --git a/packages/utils/package.json b/packages/utils/package.json
index d13c7eb2630..0897e905a23 100644
--- a/packages/utils/package.json
+++ b/packages/utils/package.json
@@ -11,7 +11,8 @@
   },
   "dependencies": {
     "@babel/runtime": "^7.4.3",
+    "p-is-promise": "^2.1.0",
     "p-lazy": "^3.0.0",
     "p-reflect": "^2.1.0"
   }
-}
+}
\ No newline at end of file
diff --git a/packages/utils/src/index.js b/packages/utils/src/index.js
index 352dd395333..8179d71bab9 100644
--- a/packages/utils/src/index.js
+++ b/packages/utils/src/index.js
@@ -1,5 +1,6 @@
 import pLazy from 'p-lazy';
 import pReflect from 'p-reflect';
+import isPromise from 'p-is-promise';
 
 export const noop = x => x;
 export const identity = noop;
@@ -153,3 +154,34 @@ export const createLazyDeferred = () => {
     },
   };
 };
+
+/**
+ * Given an array of functions which may throw a Promise when executed, we want
+ * to ensure all functions are executed, reducing any thrown Promises to a
+ * single Promise, which is itself rethrown.
+ * If no Promises are thrown, this is the equivalent of a .map
+ */
+export const captureSuspensePromises = executors => {
+  const values = [];
+  const promises = executors
+    .map(executor => {
+      try {
+        values.push(executor());
+      } catch (loadingPromiseOrError) {
+        // An actual error was thrown, so we want to bubble that up
+        if (!isPromise(loadingPromiseOrError)) {
+          throw loadingPromiseOrError;
+        }
+        // Return a Suspense promise
+        return loadingPromiseOrError;
+      }
+    })
+    .filter(Boolean);
+
+  if (promises.length) {
+    // All the suspense promises are reduced to a single promise then rethrown
+    throw Promise.all(promises);
+  }
+
+  return values;
+};
diff --git a/packages/utils/tests/utils.test.js b/packages/utils/tests/utils.test.js
index c161f48c954..b2dcee016a4 100644
--- a/packages/utils/tests/utils.test.js
+++ b/packages/utils/tests/utils.test.js
@@ -1,3 +1,4 @@
+const isPromise = require('p-is-promise');
 const {
   getType,
   escapeRegExp,
@@ -15,6 +16,7 @@ const {
   flatten,
   flatMap,
   zipObj,
+  captureSuspensePromises,
   ...utils
 } = require('../src');
 
@@ -258,4 +260,126 @@ describe('utils', () => {
       where: { AND: [{ b: 2, c: ['1', '2'] }, { d: 20, c: ['3', '4'] }] },
     });
   });
+
+  describe('captureSuspensePromises', () => {
+    test('executes all fns sync when no Promises thrown', () => {
+      const funcs = [jest.fn(), jest.fn()];
+
+      try {
+        captureSuspensePromises(funcs);
+      } catch (e) {}
+
+      expect(funcs[0]).toHaveBeenCalledTimes(1);
+      expect(funcs[1]).toHaveBeenCalledTimes(1);
+    });
+
+    test('does not throw when no executors throw', () => {
+      const funcs = [jest.fn(), jest.fn()];
+
+      try {
+        captureSuspensePromises(funcs);
+      } catch (e) {
+        expect(true).toBeFalsey();
+      }
+
+      expect(funcs[0]).toHaveBeenCalledTimes(1);
+      expect(funcs[1]).toHaveBeenCalledTimes(1);
+    });
+
+    test('executes all fns sync when a Promise is thrown', () => {
+      const funcs = [
+        jest.fn(),
+        jest.fn(() => {
+          throw Promise.resolve();
+        }),
+        jest.fn(),
+      ];
+
+      try {
+        captureSuspensePromises(funcs);
+      } catch (e) {}
+
+      expect(funcs[0]).toHaveBeenCalledTimes(1);
+      expect(funcs[1]).toHaveBeenCalledTimes(1);
+      expect(funcs[2]).toHaveBeenCalledTimes(1);
+    });
+
+    test('throws a Promise when executor throws a Promise', () => {
+      const funcs = [
+        jest.fn(),
+        jest.fn(() => {
+          throw Promise.resolve();
+        }),
+        jest.fn(),
+      ];
+
+      try {
+        captureSuspensePromises(funcs);
+        expect(true).toBeFalsey();
+      } catch (maybePromise) {
+        expect(isPromise(maybePromise)).toBeTruthy();
+      }
+    });
+
+    test('thrown Promise resolves all thrown Promises', () => {
+      const resolvers = [jest.fn(), jest.fn(), jest.fn()];
+
+      const funcs = [
+        jest.fn(() => {
+          throw new Promise(resolve => {
+            // Artificially delay this promise's resolution to the next tick
+            // which will ensure the microtask (aka: Promise) queue is fully
+            // flushed
+            process.nextTick(() => {
+              resolvers[0]();
+              resolve();
+            });
+          });
+        }),
+        jest.fn(() => {
+          throw Promise.resolve().then(resolvers[1]);
+        }),
+        jest.fn(() => {
+          throw Promise.resolve().then(resolvers[2]);
+        }),
+      ];
+
+      try {
+        captureSuspensePromises(funcs);
+      } catch (thrownPromise) {
+        return thrownPromise.then(() => {
+          expect(resolvers[0]).toHaveBeenCalledTimes(1);
+          expect(resolvers[1]).toHaveBeenCalledTimes(1);
+          expect(resolvers[2]).toHaveBeenCalledTimes(1);
+        });
+      }
+
+      expect(true).toBeFalsey();
+    });
+
+    test('throws a Error when executor throws a Error', () => {
+      const funcs = [
+        jest.fn(),
+        jest.fn(() => {
+          throw new Error();
+        }),
+        jest.fn(),
+      ];
+
+      try {
+        captureSuspensePromises(funcs);
+        expect(true).toBeFalsey();
+      } catch (maybePromise) {
+        expect(!isPromise(maybePromise)).toBeTruthy();
+      }
+    });
+
+    test('acts like .map when no promises thrown', () => {
+      const funcs = [jest.fn(() => 'foo'), jest.fn(() => 'bar')];
+
+      const result = captureSuspensePromises(funcs);
+
+      expect(result).toMatchObject(['foo', 'bar']);
+    });
+  });
 });