From eca117a4710133d30cccfcaa6d3d9c6bda53b88b Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Wed, 21 Apr 2021 12:25:11 -0400 Subject: [PATCH 1/6] Support "as" with "selector" prop defs --- .../1.0/amp-base-carousel.js | 18 ++----- extensions/amp-lightbox/1.0/base-element.js | 7 +-- .../amp-lightbox/1.0/storybook/Basic.amp.js | 4 +- .../1.0/amp-stream-gallery.js | 18 ++----- src/preact/base-element.js | 12 ++++- src/preact/slot.js | 16 ++++-- test/unit/preact/test-base-element-mapping.js | 52 +++++++++++++++++++ 7 files changed, 84 insertions(+), 43 deletions(-) diff --git a/extensions/amp-base-carousel/1.0/amp-base-carousel.js b/extensions/amp-base-carousel/1.0/amp-base-carousel.js index 77b47abe607d..498a00b36a67 100644 --- a/extensions/amp-base-carousel/1.0/amp-base-carousel.js +++ b/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}, diff --git a/extensions/amp-lightbox/1.0/base-element.js b/extensions/amp-lightbox/1.0/base-element.js index 30c01d6ece4b..690e43da6af1 100644 --- a/extensions/amp-lightbox/1.0/base-element.js +++ b/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}, }; diff --git a/extensions/amp-lightbox/1.0/storybook/Basic.amp.js b/extensions/amp-lightbox/1.0/storybook/Basic.amp.js index fd4f9daf819e..52da9e70f602 100644 --- a/extensions/amp-lightbox/1.0/storybook/Basic.amp.js +++ b/extensions/amp-lightbox/1.0/storybook/Basic.amp.js @@ -47,9 +47,7 @@ export const Default = () => {

Test

- +
diff --git a/extensions/amp-stream-gallery/1.0/amp-stream-gallery.js b/extensions/amp-stream-gallery/1.0/amp-stream-gallery.js index 1b865e897586..ad253c875838 100644 --- a/extensions/amp-stream-gallery/1.0/amp-stream-gallery.js +++ b/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}, diff --git a/src/preact/base-element.js b/src/preact/base-element.js index 00e4016a760a..b8773a6f7430 100644 --- a/src/preact/base-element.js +++ b/src/preact/base-element.js @@ -1081,7 +1081,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( @@ -1097,10 +1103,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) diff --git a/src/preact/slot.js b/src/preact/slot.js index 0667cffb92f7..650ea1e5de71 100644 --- a/src/preact/slot.js +++ b/src/preact/slot.js @@ -32,12 +32,20 @@ const EMPTY = {}; /** * @param {!Element} element * @param {string} name - * @param {!Object|undefined} props - * @return {!PreactDef.VNode} + * @param {!Object|undefined} defaultProps + * @param {boolean} as + * @return {!PreactDef.VNode|!PreactDef.FunctionalComponent} */ -export function createSlot(element, name, props) { +export function createSlot(element, name, defaultProps, as) { + /** + * @param {!Object|undefined} props + * @return {!PreactDef.VNode} + */ + function SlotWithProps(props) { + return ; + } element.setAttribute('slot', name); - return ; + return as ? SlotWithProps : SlotWithProps(); } /** diff --git a/test/unit/preact/test-base-element-mapping.js b/test/unit/preact/test-base-element-mapping.js index 64164eaa402b..9cfcd7e7a976 100644 --- a/test/unit/preact/test-base-element-mapping.js +++ b/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" >
+
{ ); }); + 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 children as prop slot array and parse attributes', () => { const {children} = lastProps; expect(children).to.have.lengthOf(2); From 94b059d503c7094bb0c3efe442faf7cc1803f48b Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Thu, 22 Apr 2021 16:04:58 -0400 Subject: [PATCH 2/6] Cache per element instance --- src/preact/slot.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/preact/slot.js b/src/preact/slot.js index 650ea1e5de71..3867a912b081 100644 --- a/src/preact/slot.js +++ b/src/preact/slot.js @@ -23,12 +23,16 @@ 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} */ +const cache = new WeakMap(); + /** * @param {!Element} element * @param {string} name @@ -37,6 +41,16 @@ const EMPTY = {}; * @return {!PreactDef.VNode|!PreactDef.FunctionalComponent} */ export function createSlot(element, name, defaultProps, as) { + element.setAttribute('slot', name); + if (!as) { + return ; + } + + const cached = cache.get(element); + if (cached && objectsEqualShallow(cached.oldProps, defaultProps)) { + return cached.component; + } + /** * @param {!Object|undefined} props * @return {!PreactDef.VNode} @@ -44,8 +58,9 @@ export function createSlot(element, name, defaultProps, as) { function SlotWithProps(props) { return ; } - element.setAttribute('slot', name); - return as ? SlotWithProps : SlotWithProps(); + cache.set(element, {oldProps: defaultProps, component: SlotWithProps}); + + return SlotWithProps; } /** From e84044eabf1e4249d1792a8494e05f274b0ea7ad Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Thu, 29 Apr 2021 13:35:12 -0400 Subject: [PATCH 3/6] Add unit tests --- src/preact/base-element.js | 35 +++++--- src/preact/slot.js | 4 +- test/unit/preact/test-base-element-mapping.js | 34 +++++++- test/unit/preact/test-slot.js | 86 ++++++++++++++++++- 4 files changed, 143 insertions(+), 16 deletions(-) diff --git a/src/preact/base-element.js b/src/preact/base-element.js index 1231ee2a273e..3f225e1a6b82 100644 --- a/src/preact/base-element.js +++ b/src/preact/base-element.js @@ -94,6 +94,7 @@ let ChildDef; /** @const {!MutationObserverInit} */ const CHILDREN_MUTATION_INIT = { childList: true, + subtree: true, }; /** @const {!MutationObserverInit} */ @@ -1269,17 +1270,7 @@ function shouldMutationBeRerendered(Ctor, m) { } // Check if the attribute is mapped to one of the properties. const props = Ctor['props']; - for (const name in props) { - const def = /** @type {!AmpElementPropDef} */ (props[name]); - if ( - m.attributeName == def.attr || - (def.attrs && def.attrs.includes(devAssert(m.attributeName))) || - matchesAttrPrefix(m.attributeName, def.attrPrefix) - ) { - return true; - } - } - return false; + return checkPropsForAttribute(props, m.attributeName); } if (type == 'childList') { return ( @@ -1289,3 +1280,25 @@ function shouldMutationBeRerendered(Ctor, m) { } return false; } + +/** + * @param {!Object} props + * @param {string} attributeName + * @return {boolean} + */ +function checkPropsForAttribute(props, attributeName) { + for (const name in props) { + const def = props[name]; + if ( + attributeName == def.attr || + (def.attrs && def.attrs.includes(devAssert(attributeName))) || + matchesAttrPrefix(attributeName, def.attrPrefix) + ) { + return true; + } + if (def.props) { + return checkPropsForAttribute(def.props, attributeName); + } + } + return false; +} diff --git a/src/preact/slot.js b/src/preact/slot.js index 3867a912b081..338471f62278 100644 --- a/src/preact/slot.js +++ b/src/preact/slot.js @@ -37,10 +37,10 @@ const cache = new WeakMap(); * @param {!Element} element * @param {string} name * @param {!Object|undefined} defaultProps - * @param {boolean} as + * @param {boolean|undefined} as * @return {!PreactDef.VNode|!PreactDef.FunctionalComponent} */ -export function createSlot(element, name, defaultProps, as) { +export function createSlot(element, name, defaultProps, as = false) { element.setAttribute('slot', name); if (!as) { return ; diff --git a/test/unit/preact/test-base-element-mapping.js b/test/unit/preact/test-base-element-mapping.js index 9cfcd7e7a976..0372f542b9d2 100644 --- a/test/unit/preact/test-base-element-mapping.js +++ b/test/unit/preact/test-base-element-mapping.js @@ -700,6 +700,35 @@ describes.realWin('PreactBaseElement', spec, (env) => { ); }); + 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', + }); + + const node = element.querySelector('[special3]'); + node.setAttribute('value-with-def', 'CUSTOM'); + + 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); @@ -1196,7 +1225,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', { @@ -1209,7 +1238,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]; @@ -1222,6 +1251,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 () => { diff --git a/test/unit/preact/test-slot.js b/test/unit/preact/test-slot.js index 2e79c92ca835..4082e34fe615 100644 --- a/test/unit/preact/test-slot.js +++ b/test/unit/preact/test-slot.js @@ -21,7 +21,7 @@ import { CanRender, LoadingProp, } from '../../../src/context/contextprops'; -import {Slot, useSlotContext} from '../../../src/preact/slot'; +import {Slot, createSlot, useSlotContext} from '../../../src/preact/slot'; import {WithAmpContext} from '../../../src/preact/context'; import {createElementWithAttributes} from '../../../src/dom'; import {createRef, useLayoutEffect, useRef} from '../../../src/preact'; @@ -344,3 +344,87 @@ describes.realWin('Slot mount/unmount', {}, (env) => { }); }); }); + +describes.realWin('createSlot', {}, (env) => { + let win, doc; + + beforeEach(() => { + win = env.win; + doc = win.document; + }); + + it('should create slot and set corresponding slot attr', () => { + const element = doc.createElement('div'); + const slot = createSlot(element, 'element'); + expect(element.getAttribute('slot')).to.equal('element'); + expect(slot.type).to.equal(Slot); + expect(slot.props).to.deep.equal({'name': 'element'}); + }); + + it('should create slot with props', () => { + const element = doc.createElement('div'); + const slot = createSlot(element, 'element', {'propA': true}); + expect(element.getAttribute('slot')).to.equal('element'); + expect(slot.type).to.equal(Slot); + expect(slot.props).to.deep.equal({'name': 'element', 'propA': true}); + }); + + it('should create slot function and set corresponding slot attr', () => { + const element = doc.createElement('div'); + const slotComp = createSlot(element, 'element', {}, /* as */ true); + expect(element.getAttribute('slot')).to.equal('element'); + + expect(typeof slotComp).to.equal('function'); + expect(slotComp.name).to.equal('SlotWithProps'); + + const slot = slotComp(); + expect(slot.type).to.equal(Slot); + expect(slot.props).to.deep.equal({'name': 'element'}); + }); + + it('should create slot function with props', () => { + const element = doc.createElement('div'); + const slotComp = createSlot( + element, + 'element', + {'propA': true}, + /* as */ true + ); + expect(element.getAttribute('slot')).to.equal('element'); + + expect(typeof slotComp).to.equal('function'); + expect(slotComp.name).to.equal('SlotWithProps'); + + const slot = slotComp(); + expect(slot.type).to.equal(Slot); + expect(slot.props).to.deep.equal({'name': 'element', 'propA': true}); + }); + + it('should return cached slot function', () => { + const element = doc.createElement('div'); + const slotComp = createSlot(element, 'element', {}, /* as */ true); + const slotComp2 = createSlot(element, 'element', {}, /* as */ true); + expect(slotComp2).to.deep.equal(slotComp); + }); + + it('should update cached slot function with new defaultProps', () => { + const element = doc.createElement('div'); + const slotComp = createSlot(element, 'element', {}, /* as */ true); + const slotComp2 = createSlot( + element, + 'element', + {'propA': true}, + /* as */ true + ); + expect(slotComp2).not.to.deep.equal(slotComp); + }); + + it('should return unique slot functions for unique elements', () => { + const element = doc.createElement('div'); + const slotComp = createSlot(element, 'element', {}, /* as */ true); + + const element2 = doc.createElement('div'); + const slotComp2 = createSlot(element2, 'element', {}, /* as */ true); + expect(slotComp2).not.to.deep.equal(slotComp); + }); +}); From c52ff81d9e47d2f5fc4151acc833c522591b8e39 Mon Sep 17 00:00:00 2001 From: Caroline Liu <10456171+caroqliu@users.noreply.github.com> Date: Thu, 29 Apr 2021 16:45:52 -0400 Subject: [PATCH 4/6] Update test/unit/preact/test-slot.js Co-authored-by: Justin Ridgewell --- test/unit/preact/test-slot.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/unit/preact/test-slot.js b/test/unit/preact/test-slot.js index 4082e34fe615..1a4763f50b11 100644 --- a/test/unit/preact/test-slot.js +++ b/test/unit/preact/test-slot.js @@ -409,7 +409,12 @@ describes.realWin('createSlot', {}, (env) => { it('should update cached slot function with new defaultProps', () => { const element = doc.createElement('div'); - const slotComp = createSlot(element, 'element', {}, /* as */ true); + const slotComp = createSlot( + element, + 'element', + {'propA': false}, + /* as */ true + ); const slotComp2 = createSlot( element, 'element', From e5ef884b084cbb86c05c879758946146e44e2cb0 Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Thu, 29 Apr 2021 17:16:11 -0400 Subject: [PATCH 5/6] Do not observe subtree mutations --- src/preact/base-element.js | 35 ++++++------------- test/unit/preact/test-base-element-mapping.js | 8 +++-- 2 files changed, 17 insertions(+), 26 deletions(-) diff --git a/src/preact/base-element.js b/src/preact/base-element.js index 1685d5a3ad2c..8b3fe2bb4271 100644 --- a/src/preact/base-element.js +++ b/src/preact/base-element.js @@ -95,7 +95,6 @@ let ChildDef; /** @const {!MutationObserverInit} */ const CHILDREN_MUTATION_INIT = { childList: true, - subtree: true, }; /** @const {!MutationObserverInit} */ @@ -1271,7 +1270,17 @@ function shouldMutationBeRerendered(Ctor, m) { } // Check if the attribute is mapped to one of the properties. const props = Ctor['props']; - return checkPropsForAttribute(props, m.attributeName); + for (const name in props) { + const def = /** @type {!AmpElementPropDef} */ (props[name]); + if ( + m.attributeName == def.attr || + (def.attrs && def.attrs.includes(devAssert(m.attributeName))) || + matchesAttrPrefix(m.attributeName, def.attrPrefix) + ) { + return true; + } + } + return false; } if (type == 'childList') { return ( @@ -1281,25 +1290,3 @@ function shouldMutationBeRerendered(Ctor, m) { } return false; } - -/** - * @param {!Object} props - * @param {string} attributeName - * @return {boolean} - */ -function checkPropsForAttribute(props, attributeName) { - for (const name in props) { - const def = props[name]; - if ( - attributeName == def.attr || - (def.attrs && def.attrs.includes(devAssert(attributeName))) || - matchesAttrPrefix(attributeName, def.attrPrefix) - ) { - return true; - } - if (def.props) { - return checkPropsForAttribute(def.props, attributeName); - } - } - return false; -} diff --git a/test/unit/preact/test-base-element-mapping.js b/test/unit/preact/test-base-element-mapping.js index 0372f542b9d2..41dbdee7d14f 100644 --- a/test/unit/preact/test-base-element-mapping.js +++ b/test/unit/preact/test-base-element-mapping.js @@ -711,8 +711,12 @@ describes.realWin('PreactBaseElement', spec, (env) => { name: 'i-amphtml-specialAs', }); - const node = element.querySelector('[special3]'); - node.setAttribute('value-with-def', 'CUSTOM'); + // 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; From a01e1b7e479fb8cc499211fa31b28b53ba7072af Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Fri, 30 Apr 2021 09:21:12 -0400 Subject: [PATCH 6/6] (empty)