Bento: expose Imperative API object (ampproject#31661)

* Implement whenUpgraded for Bento Imperative APIs

* Need to wait for the impl, then the API

* Expose API on AMP Elements

Just links to the Impl

* Resolve promise with the wrapped API, not the ref

* Fix incorrect this, use ref holder directly

* Tests and fixes

* Fix type

* Move getApi to BaseElement

* Remove createRef use

* Add private annotations

* Throw err if API shape changes

* Fix jsdoc

* Remove .only test

* Test throw when API surface changes

* Use devAssert

* Type updates

* Add CustomElement.getApi()

build-system/eslint-rules/no-array-destructuring.js

@@ -49,7 +49,10 @@ module.exports = function (context) {
       if (computed) {
         return false;
       }
-      if (object.type !== 'Identifier' || object.name !== 'preact') {
+      if (
+        object.type !== 'Identifier' ||
+        object.name.toLowerCase() !== 'preact'
+      ) {
         return false;
       }
       if (property.type !== 'Identifier' || !property.name.startsWith('use')) {

src/base-element.js

@@ -954,4 +954,15 @@ export class BaseElement {
   user() {
     return user(this.element);
   }
+
+  /**
+   * Returns this BaseElement instance. This is equivalent to Bento's
+   * imperative API object, since this is where we define the element's custom
+   * APIs.
+   *
+   * @return {!Promise<!Object>}
+   */
+  getApi() {
+    return this;
+  }
 }

src/custom-element.js

@@ -1074,6 +1074,17 @@ function createBaseCustomElementClass(win) {
       return waitFor.then(() => this.implementation_);
     }
 
+    /**
+     * Returns the object which holds the API surface (the thing we add the
+     * custom methods/properties onto). In Bento, this is the imperative API
+     * object. In AMP, this is the BaseElement instance.
+     *
+     * @return {!Promise<!Object>}
+     */
+    getApi() {
+      return this.getImpl().then((impl) => impl.getApi());
+    }
+
     /**
      * Returns the layout of the element.
      * @return {!Layout}

src/preact/base-element.js

@@ -38,12 +38,12 @@ import {
   matches,
   parseBooleanAttribute,
 } from '../dom';
-import {createRef, hydrate, render} from './index';
 import {dashToCamelCase} from '../string';
 import {devAssert} from '../log';
-import {dict, hasOwn} from '../utils/object';
+import {dict, hasOwn, map} from '../utils/object';
 import {getDate} from '../utils/date';
 import {getMode} from '../mode';
+import {hydrate, render} from './index';
 import {installShadowStyle} from '../shadow-embed';
 import {isLayoutSizeDefined} from '../layout';
 import {sequentialIdGenerator} from '../utils/id-generator';
@@ -161,8 +161,27 @@ export class PreactBaseElement extends AMP.BaseElement {
       notify: () => this.mutateElement(() => {}),
     };
 
-    /** @private {{current: ?API_TYPE}} */
-    this.ref_ = createRef();
+    /** @private {?API_TYPE} */
+    this.apiWrapper_ = null;
+
+    /** @private {?API_TYPE} */
+    this.currentRef_ = null;
+
+    /** @param {?API_TYPE|null} current */
+    this.refSetter_ = (current) => {
+      // The API shape **must** be consistent.
+      if (current !== null) {
+        if (this.apiWrapper_) {
+          this.checkApiWrapper_(current);
+        } else {
+          this.initApiWrapper_(current);
+        }
+      }
+      this.currentRef_ = current;
+    };
+
+    /** @type {?Deferred<!API_TYPE>} */
+    this.deferredApi_ = null;
 
     /** @private {?Array} */
     this.contextValues_ = null;
@@ -301,7 +320,7 @@ export class PreactBaseElement extends AMP.BaseElement {
     this.mutateProps(dict({'loading': Loading.EAGER}));
 
     // Check if the element has already been loaded.
-    const api = this.ref_.current;
+    const api = this.currentRef_;
     if (api && api['complete']) {
       return Promise.resolve();
     }
@@ -356,7 +375,7 @@ export class PreactBaseElement extends AMP.BaseElement {
    * @protected
    */
   api() {
-    return devAssert(this.ref_.current);
+    return devAssert(this.currentRef_);
   }
 
   /**
@@ -552,7 +571,7 @@ export class PreactBaseElement extends AMP.BaseElement {
     const props = collectProps(
       Ctor,
       this.element,
-      this.ref_,
+      this.refSetter_,
       this.defaultProps_,
       this.mediaQueryProps_
     );
@@ -617,6 +636,119 @@ export class PreactBaseElement extends AMP.BaseElement {
     }
     return this.defaultProps_[prop];
   }
+
+  /**
+   * Returns reference to upgraded imperative API object, as in React's
+   * useImperativeHandle.
+   *
+   * @return {!Promise<!API_TYPE>}
+   * @override
+   */
+  getApi() {
+    const api = this.apiWrapper_;
+    if (api) {
+      return Promise.resolve(api);
+    }
+    if (!this.deferredApi_) {
+      this.deferredApi_ = new Deferred();
+    }
+    return this.deferredApi_.promise;
+  }
+
+  /**
+   * Creates a wrapper around a Preact ref. The API surface exposed by this ref
+   * **must** be consistent accross all rerenders.
+   *
+   * This wrapper is necessary because every time React rerenders, it creates
+   * (depending on deps checking) a new imperative handle and sets that to
+   * `ref.current`. So if we ever returned `ref.current` directly, it could go
+   * stale by the time its actually used.
+   *
+   * @param {!API_TYPE} current
+   * @private
+   */
+  initApiWrapper_(current) {
+    const api = map();
+    const keys = Object.keys(current);
+    for (let i = 0; i < keys.length; i++) {
+      const key = keys[i];
+      wrapRefProperty(this, api, key);
+    }
+    this.apiWrapper_ = api;
+    if (this.deferredApi_) {
+      this.deferredApi_.resolve(api);
+      this.deferredApi_ = null;
+    }
+  }
+
+  /**
+   * Verifies that every Preact render exposes the same API surface as the previous render.
+   * If it does not, the API wrapper is syncrhonized.
+   *
+   * @param {!API_TYPE} current
+   * @private
+   */
+  checkApiWrapper_(current) {
+    if (!getMode().localDev) {
+      return;
+    }
+    const api = this.apiWrapper_;
+    const newKeys = Object.keys(current);
+    for (let i = 0; i < newKeys.length; i++) {
+      const key = newKeys[i];
+      devAssert(
+        hasOwn(api, key),
+        'Inconsistent Bento API shape: imperative API gained a "%s" key for %s',
+        key,
+        this.element
+      );
+    }
+    const oldKeys = Object.keys(api);
+    for (let i = 0; i < oldKeys.length; i++) {
+      const key = oldKeys[i];
+      devAssert(
+        hasOwn(current, key),
+        'Inconsistent Bento API shape: imperative API lost a "%s" key for %s',
+        key,
+        this.element
+      );
+    }
+  }
+}
+
+/**
+ * @param {tyepof PreactBaseElement} baseElement
+ * @param {!Object} api
+ * @param {string} key
+ */
+function wrapRefProperty(baseElement, api, key) {
+  Object.defineProperty(api, key, {
+    configurable: true,
+
+    get() {
+      return baseElement.currentRef_[key];
+    },
+
+    set(v) {
+      baseElement.currentRef_[key] = v;
+    },
+  });
+}
+
+/**
+ * Returns the upgraded imperative API object, once Preact has actually mounted.
+ *
+ * This technically works with both Bento and Legacy components, returning the
+ * BaseElement instance in the later case.
+ *
+ * @param {!Element} el
+ * @return {!Promise<!Object>}
+ */
+export function whenUpgraded(el) {
+  return el.ownerDocument.defaultView.customElements
+    .whenDefined(el.localName)
+    .then(() => el.getImpl())
+    .then((impl) => impl.getApi());
 }
 
 // Ideally, these would be Static Class Fields. But Closure can't even.

src/preact/index.js

@@ -83,7 +83,7 @@ export function createContext(value) {
 }
 
 // Defines the type interfaces for the approved Preact Hooks APIs.
-// TODO: useReducer, useImperativeHandle, useDebugValue, useErrorBoundary
+// TODO: useReducer, useDebugValue, useErrorBoundary
 
 /**
  * @param {S|function():S} initial

test/unit/preact/test-base-element-runtime.js

@@ -16,8 +16,12 @@
 
 import * as Preact from '../../../src/preact/index';
 import {CanRender} from '../../../src/contextprops';
-import {PreactBaseElement} from '../../../src/preact/base-element';
+import {
+  PreactBaseElement,
+  whenUpgraded,
+} from '../../../src/preact/base-element';
 import {Slot} from '../../../src/preact/slot';
+import {forwardRef} from '../../../src/preact/compat';
 import {htmlFor} from '../../../src/static-template';
 import {removeElement} from '../../../src/dom';
 import {subscribe} from '../../../src/context';
@@ -206,3 +210,129 @@ describes.realWin('PreactBaseElement', {amp: true}, (env) => {
     });
   });
 });
+
+describes.realWin('whenUpgraded', {amp: true}, (env) => {
+  let win;
+  let doc;
+  let Impl;
+  let Component;
+
+  beforeEach(() => {
+    win = env.win;
+    doc = win.document;
+
+    Impl = class extends PreactBaseElement {
+      isLayoutSupported() {
+        return true;
+      }
+    };
+    Component = (props, ref) => {
+      Preact.useImperativeHandle(ref, () => {
+        return {key: true};
+      });
+    };
+    Impl['Component'] = forwardRef(Component);
+    Impl['children'] = {};
+    Impl['loadable'] = true;
+  });
+
+  it('waits for CE definition', async () => {
+    const el = doc.createElement('amp-preact');
+    doc.body.appendChild(el);
+    const p = whenUpgraded(el);
+    upgradeOrRegisterElement(win, 'amp-preact', Impl);
+    el.build();
+
+    const api = await p;
+    expect(api.key).to.be.true;
+  });
+
+  it('waits for build', async () => {
+    const el = doc.createElement('amp-preact');
+    doc.body.appendChild(el);
+    upgradeOrRegisterElement(win, 'amp-preact', Impl);
+    const p = whenUpgraded(el);
+    el.build();
+
+    const api = await p;
+    expect(api.key).to.be.true;
+  });
+
+  it('resolves after mount', async () => {
+    const el = doc.createElement('amp-preact');
+    doc.body.appendChild(el);
+    upgradeOrRegisterElement(win, 'amp-preact', Impl);
+    await el.build();
+    const p = whenUpgraded(el);
+
+    const api = await p;
+    expect(api.key).to.be.true;
+  });
+
+  it('wraps API to preserve object identity accross rerenders', async () => {
+    let imperativeApi;
+    let setState;
+    Component = env.sandbox.stub().callsFake((props, ref) => {
+      const [state, set] = Preact.useState(0);
+      setState = set;
+      Preact.useImperativeHandle(ref, () => {
+        return (imperativeApi = {state});
+      });
+    });
+    Impl['Component'] = forwardRef(Component);
+    const el = doc.createElement('amp-preact');
+    doc.body.appendChild(el);
+    upgradeOrRegisterElement(win, 'amp-preact', Impl);
+    const p = whenUpgraded(el);
+    el.build();
+
+    const api = await p;
+    expect(api).not.to.equal(imperativeApi);
+    expect(api.state).to.equal(0);
+
+    const current = Component.callCount;
+    setState(1);
+    await waitFor(
+      () => Component.callCount > current,
+      'rerender after setState'
+    );
+
+    const api2 = await whenUpgraded(el);
+    expect(api2).to.equal(api);
+    expect(api.state).to.equal(1);
+  });
+
+  it('throws when API surface changes after rerender', async () => {
+    let setState;
+    Component = env.sandbox.stub().callsFake((props, ref) => {
+      const [api, set] = Preact.useState({
+        first: true,
+      });
+      setState = set;
+      Preact.useImperativeHandle(ref, () => {
+        return api;
+      });
+    });
+    Impl['Component'] = forwardRef(Component);
+    const el = doc.createElement('amp-preact');
+    doc.body.appendChild(el);
+    upgradeOrRegisterElement(win, 'amp-preact', Impl);
+    const p = whenUpgraded(el);
+    el.build();
+
+    const api = await p;
+    expect(api.first).to.be.true;
+    expect(el).not.to.have.display('none');
+
+    const current = Component.callCount;
+    setState({
+      second: true,
+    });
+    await waitFor(
+      () => Component.callCount > current,
+      'rerender after setState'
+    );
+
+    expect(el).to.have.class('i-amphtml-error');
+  });
+});