Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PreactBaseElement: Support returning a function for "selector" prop defs #33935

Merged
merged 8 commits into from
Apr 30, 2021
Merged

PreactBaseElement: Support returning a function for "selector" prop defs #33935

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eca117a
Support "as" with "selector" prop defs
caroqliu Apr 21, 2021
94b059d
Cache per element instance
caroqliu Apr 22, 2021
7028698
Merge branch 'main' into selector-props-fn
caroqliu Apr 28, 2021
e84044e
Add unit tests
caroqliu Apr 29, 2021
c43d3d9
Merge branch 'main' into selector-props-fn
caroqliu Apr 29, 2021
c52ff81
Update test/unit/preact/test-slot.js
caroqliu Apr 29, 2021
e5ef884
Do not observe subtree mutations
caroqliu Apr 29, 2021
a01e1b7
(empty)
caroqliu Apr 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 4 additions & 14 deletions extensions/amp-base-carousel/1.0/amp-base-carousel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 */
Expand All @@ -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},
Expand Down
7 changes: 1 addition & 6 deletions extensions/amp-lightbox/1.0/base-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,6 @@ export class BaseElement extends PreactBaseElement {
this.removeAsContainer();
}

/** @override */
updatePropsForRendering(props) {
props['closeButtonAs'] = () => props['closeButton'];
}

/** @private */
beforeOpen_() {
this.open_ = true;
Expand Down Expand Up @@ -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},
};

Expand Down
4 changes: 1 addition & 3 deletions extensions/amp-lightbox/1.0/storybook/Basic.amp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
18 changes: 4 additions & 14 deletions extensions/amp-stream-gallery/1.0/amp-stream-gallery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
}
}
}

/**
Expand Down Expand Up @@ -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},
Expand Down
12 changes: 10 additions & 2 deletions src/preact/base-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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)
Expand Down
31 changes: 27 additions & 4 deletions src/preact/slot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually has the same issue mentioned in the other thread. Here, SlotWithProps function is being recreated every time we render, which means the Component identity changes every time. This will cause the <slot> DOM node to be removed, a new one created, and then inserted during every diff.

Now that we're operating on the PreactBaseElement level, we can actually cache this per the element instance. Eg:

const cache: WeakMap<Element, {oldDefauls: (!Object|undefined), component: Component}> = new WeakMap();

export function createSlot(element, name, defaultProps, as) {
  element.setAttribute('slot', name);
  if (!as) {
    return <Slot {...(defaultProps || EMPTY)} name={name} />;
  }

  const cached = cache.get(element);
  if (cached && shallowEquals(cached.oldProps, defaultProps)) {
    return cached.component;
  }
  function SlotWithProps(props) {
    return <Slot {...(defaultProps || EMPTY)} name={name} {...props} />;
  }
  cache.set(element, { oldProps: defaultProps, component: SlotWithProps });

  return SlotWithProps;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying! Updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'd like to see a unit test verifying the <slot> DOM node is the same after a rerender, and possibly one asserting that it is new after the defaultProps changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get the second case, we will have to enable both deep prop def checking in shouldMutationBeRerendered and include subtree: true in CHILDREN_MUTATION_INIT -- wanted to confirm this is OK.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid it with:

PBE['props'] = {
  next: {
    selector: '…',
    props: {
      bar: {attr: 'bar'},
    },
  },
  foo: {
    attr: 'foo',
  },
};

// Mutate slot prop, but this won't trigger a rerender
el.querySelector('next').setAttribute('bar', 'other');

// Mutate an observed attr to trigger rerender
el.setAttribute('foo', 'foooey');

If it's more complicated than that, I think it's ok with just the unit tests you wrote.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, that works. I removed the changes to the mutation observer then. Thanks!

return <Slot {...(defaultProps || EMPTY)} name={name} {...props} />;
}
cache.set(element, {oldProps: defaultProps, component: SlotWithProps});

return SlotWithProps;
}

/**
Expand Down
90 changes: 88 additions & 2 deletions test/unit/preact/test-base-element-mapping.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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': {
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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', {
Expand All @@ -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];

Expand All @@ -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 () => {
Expand Down