PreactBaseElement: Support returning a function for "selector" prop d…

…efs (ampproject#33935)

* Support "as" with "selector" prop defs

* Cache per element instance

* Add unit tests

* Update test/unit/preact/test-slot.js

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>

* Do not observe subtree mutations

* (empty)

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>

extensions/amp-base-carousel/1.0/amp-base-carousel.js

@@ -21,7 +21,6 @@ import {CSS} from '../../../build/amp-base-carousel-1.0.css';
 import {CarouselContextProp} from './carousel-props';
 import {PreactBaseElement} from '../../../src/preact/base-element';
 import {Services} from '../../../src/services';
-import {cloneElement} from '../../../src/preact';
 import {createCustomEvent} from '../../../src/event-helper';
 import {dict} from '../../../src/core/types/object';
 import {dispatchCustomEvent} from '../../../src/dom';
@@ -85,17 +84,6 @@ class AmpBaseCarousel extends PreactBaseElement {
       this.api().goToSlide(slide);
     }
   }
-
-  /** @override */
-  updatePropsForRendering(props) {
-    const {arrowPrev, arrowNext} = props;
-    if (arrowPrev) {
-      props['arrowPrevAs'] = (props) => cloneElement(arrowPrev, props);
-    }
-    if (arrowNext) {
-      props['arrowNextAs'] = (props) => cloneElement(arrowNext, props);
-    }
-  }
 }
 
 /** @override */
@@ -107,13 +95,15 @@ AmpBaseCarousel['layoutSizeDefined'] = true;
 /** @override */
 AmpBaseCarousel['props'] = {
   'advanceCount': {attr: 'advance-count', type: 'number', media: true},
-  'arrowPrev': {
+  'arrowPrevAs': {
     selector: '[slot="prev-arrow"]',
     single: true,
+    as: true,
   },
-  'arrowNext': {
+  'arrowNextAs': {
     selector: '[slot="next-arrow"]',
     single: true,
+    as: true,
   },
   'autoAdvance': {attr: 'auto-advance', type: 'boolean', media: true},
   'autoAdvanceCount': {attr: 'auto-advance-count', type: 'number', media: true},

extensions/amp-lightbox/1.0/base-element.js

@@ -45,11 +45,6 @@ export class BaseElement extends PreactBaseElement {
     this.removeAsContainer();
   }
 
-  /** @override */
-  updatePropsForRendering(props) {
-    props['closeButtonAs'] = () => props['closeButton'];
-  }
-
   /** @private */
   beforeOpen_() {
     this.open_ = true;
@@ -95,7 +90,7 @@ BaseElement['Component'] = Lightbox;
 /** @override */
 BaseElement['props'] = {
   'animation': {attr: 'animation', media: true, default: 'fade-in'},
-  'closeButton': {selector: '[slot="close-button"]', single: true},
+  'closeButtonAs': {selector: '[slot="close-button"]', single: true, as: true},
   'children': {passthrough: true},
 };
 

extensions/amp-lightbox/1.0/storybook/Basic.amp.js

@@ -46,9 +46,7 @@ export const Default = () => {
       <div style="height: 300px;">
         <amp-lightbox id="lightbox" layout="nodisplay" animation={animation}>
           <p>Test</p>
-          <button slot="close-button" on="tap:lightbox.close">
-            Close
-          </button>
+          <button slot="close-button">Close</button>
         </amp-lightbox>
         <div class="buttons">
           <button on="tap:lightbox">Open</button>

extensions/amp-stream-gallery/1.0/amp-stream-gallery.js

@@ -21,7 +21,6 @@ import {CSS as GALLERY_CSS} from './stream-gallery.jss';
 import {PreactBaseElement} from '../../../src/preact/base-element';
 import {Services} from '../../../src/services';
 import {StreamGallery} from './stream-gallery';
-import {cloneElement} from '../../../src/preact';
 import {createCustomEvent} from '../../../src/event-helper';
 import {dict} from '../../../src/core/types/object';
 import {dispatchCustomEvent} from '../../../src/dom';
@@ -62,17 +61,6 @@ class AmpStreamGallery extends PreactBaseElement {
     );
     return super.isLayoutSupported(layout);
   }
-
-  /** @override */
-  updatePropsForRendering(props) {
-    const {arrowPrev, arrowNext} = props;
-    if (arrowPrev) {
-      props['arrowPrevAs'] = (props) => cloneElement(arrowPrev, props);
-    }
-    if (arrowNext) {
-      props['arrowNextAs'] = (props) => cloneElement(arrowNext, props);
-    }
-  }
 }
 
 /**
@@ -109,13 +97,15 @@ AmpStreamGallery['layoutSizeDefined'] = true;
 
 /** @override */
 AmpStreamGallery['props'] = {
-  'arrowPrev': {
+  'arrowPrevAs': {
     selector: '[slot="prev-arrow"]',
     single: true,
+    as: true,
   },
-  'arrowNext': {
+  'arrowNextAs': {
     selector: '[slot="next-arrow"]',
     single: true,
+    as: true,
   },
   'controls': {attr: 'controls', type: 'string', media: true},
   'extraSpace': {attr: 'extra-space', type: 'string', media: true},

src/preact/base-element.js

@@ -1082,7 +1082,13 @@ function parsePropDefs(Ctor, props, propDefs, element, mediaQueryProps) {
         continue;
       }
       const def = propDefs[match];
-      const {single, name = match, clone, props: slotProps = {}} = def;
+      const {
+        as = false,
+        single,
+        name = match,
+        clone,
+        props: slotProps = {},
+      } = def;
       devAssert(clone || Ctor['usesShadowDom']);
       const parsedSlotProps = {};
       parsePropDefs(
@@ -1098,10 +1104,12 @@ function parsePropDefs(Ctor, props, propDefs, element, mediaQueryProps) {
         props[name] = createSlot(
           childElement,
           childElement.getAttribute('slot') || `i-amphtml-${name}`,
-          parsedSlotProps
+          parsedSlotProps,
+          as
         );
       } else {
         const list = props[name] || (props[name] = []);
+        devAssert(!as);
         list.push(
           clone
             ? createShallowVNodeCopy(childElement)

src/preact/slot.js

@@ -24,21 +24,44 @@ import {
   pauseAll,
   unmountAll,
 } from '../utils/resource-container-helper';
+import {objectsEqualShallow} from '../core/types/object';
 import {rediscoverChildren, removeProp, setProp} from '../context';
 import {useAmpContext} from './context';
 import {useEffect, useLayoutEffect, useRef} from './index';
 
 const EMPTY = {};
 
+/** @const {WeakMap<Element, {oldDefauls: (!Object|undefined), component: Component}>} */
+const cache = new WeakMap();
+
 /**
  * @param {!Element} element
  * @param {string} name
- * @param {!Object|undefined} props
- * @return {!PreactDef.VNode}
+ * @param {!Object|undefined} defaultProps
+ * @param {boolean|undefined} as
+ * @return {!PreactDef.VNode|!PreactDef.FunctionalComponent}
  */
-export function createSlot(element, name, props) {
+export function createSlot(element, name, defaultProps, as = false) {
   element.setAttribute('slot', name);
-  return <Slot {...(props || EMPTY)} name={name} />;
+  if (!as) {
+    return <Slot {...(defaultProps || EMPTY)} name={name} />;
+  }
+
+  const cached = cache.get(element);
+  if (cached && objectsEqualShallow(cached.oldProps, defaultProps)) {
+    return cached.component;
+  }
+
+  /**
+   * @param {!Object|undefined} props
+   * @return {!PreactDef.VNode}
+   */
+  function SlotWithProps(props) {
+    return <Slot {...(defaultProps || EMPTY)} name={name} {...props} />;
+  }
+  cache.set(element, {oldProps: defaultProps, component: SlotWithProps});
+
+  return SlotWithProps;
 }
 
 /**

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

@@ -561,6 +561,19 @@ describes.realWin('PreactBaseElement', spec, (env) => {
           selector: '[special2]',
           single: true,
         },
+        'specialAs': {
+          selector: '[special3]',
+          props: {
+            'noValue': {attr: 'no-value'},
+            'valueWithDef': {attr: 'value-with-def', default: 'DEFAULT'},
+            'propA': {attr: 'prop-a'},
+            'minFontSize': {attr: 'min-font-size', type: 'number'},
+            'disabled': {attr: 'disabled', type: 'boolean'},
+            'enabled': {attr: 'enabled', type: 'boolean'},
+          },
+          single: true,
+          as: true,
+        },
         'children': {
           props: {
             'boolDefTrue': {
@@ -589,6 +602,13 @@ describes.realWin('PreactBaseElement', spec, (env) => {
             disabled
             unknown="1"
           ></div>
+          <div
+            special3
+            prop-a="A"
+            min-font-size="72"
+            disabled
+            unknown="1"
+          ></div>
           <div
             id="child1"
             part-a="A"
@@ -648,6 +668,71 @@ describes.realWin('PreactBaseElement', spec, (env) => {
       );
     });
 
+    it('should pass children as functional prop slot for single-element mapping with "as" and parse attributes', () => {
+      const {specialAs: Comp} = lastProps;
+      expect(typeof Comp).to.equal('function');
+      expect(Comp.name).to.equal('SlotWithProps');
+
+      const special3 = Comp();
+      expect(special3.props).to.deep.equal({
+        valueWithDef: 'DEFAULT',
+        propA: 'A',
+        minFontSize: 72,
+        disabled: true,
+        name: 'i-amphtml-specialAs',
+      });
+
+      const special3WithProps = Comp({
+        'aria-disabled': 'false',
+        disabled: false,
+      });
+      expect(special3WithProps.props).to.deep.equal({
+        valueWithDef: 'DEFAULT',
+        propA: 'A',
+        minFontSize: 72,
+        name: 'i-amphtml-specialAs',
+        'aria-disabled': 'false',
+        disabled: false,
+      });
+
+      expect(element.querySelector('[special3]').slot).to.equal(
+        'i-amphtml-specialAs'
+      );
+    });
+
+    it('should pass new functional prop slot for "as" on mutation', async () => {
+      const {specialAs: prevComp} = lastProps;
+      const prevSpecial3 = prevComp();
+      expect(prevSpecial3.props).to.deep.equal({
+        valueWithDef: 'DEFAULT',
+        propA: 'A',
+        minFontSize: 72,
+        disabled: true,
+        name: 'i-amphtml-specialAs',
+      });
+
+      // Mutate slot prop, but this won't trigger a rerender
+      element
+        .querySelector('[special3]')
+        .setAttribute('value-with-def', 'CUSTOM');
+      // Mutate an observed attr to trigger rerender
+      element.setAttribute('prop-a', 'B');
+
+      await waitFor(() => component.callCount > 1, 'component re-rendered');
+      expect(component).to.be.calledTwice;
+
+      const {specialAs: Comp} = lastProps;
+      expect(Comp).not.to.deep.equal(prevComp);
+      const special3 = Comp();
+      expect(special3.props).to.deep.equal({
+        valueWithDef: 'CUSTOM',
+        propA: 'A',
+        minFontSize: 72,
+        disabled: true,
+        name: 'i-amphtml-specialAs',
+      });
+    });
+
     it('should pass children as prop slot array and parse attributes', () => {
       const {children} = lastProps;
       expect(children).to.have.lengthOf(2);
@@ -1144,7 +1229,7 @@ describes.realWin('PreactBaseElement', spec, (env) => {
 
     it('should rerender on new children', async () => {
       await waitFor(() => component.callCount > 0, 'component rendered');
-      const {children: prevChildren} = lastProps;
+      const {children: prevChildren, specialAs: prevSpecialAs} = lastProps;
       expect(prevChildren).to.have.lengthOf(1);
 
       const newChild = createElementWithAttributes(doc, 'div', {
@@ -1157,7 +1242,7 @@ describes.realWin('PreactBaseElement', spec, (env) => {
       await waitFor(() => component.callCount > 1, 'component re-rendered');
       expect(component).to.be.calledTwice;
 
-      const {children} = lastProps;
+      const {children, specialAs} = lastProps;
       expect(children).to.have.lengthOf(1);
       const child = children[0];
 
@@ -1170,6 +1255,7 @@ describes.realWin('PreactBaseElement', spec, (env) => {
       expect(element.querySelector('#child1').slot).to.equal('');
       expect(element.querySelector('#child2').slot).to.equal('');
       expect(element.textContent).to.contain('text (should be passed through)');
+      expect(specialAs).to.deep.equal(prevSpecialAs);
     });
 
     it('should rerender on text change', async () => {