Simplify AMP/Bento Class Hierarchies

[samouri]

Objectives

1. Simplify the class hierarchies of AMP 1.0 and Bento Components
2. Ensure proper layering of AMP/Bento s.t. the Bento side is devoid of AMP concepts

Current

• AMP Hierarchy: AmpFitText.js > FitTextBaseElement > PreactBaseElement > AMP.BaseElement
• Bento Hierarchy: FitTextBaseElement > PreactBaseElement > CeBaseElement

Proposed State

changes

• We can remove CeBaseElement from Bento by extracting the CustomElement function and also moving all of the HTMLElement CE Mappings into PreactBaseElement.
• We can extract all of the AMP-specific logic from PreactBaseElement into AMPPreactBaseElement.

new hierarchies

• AMP Hierarchy: AmpFitText > AMPPreactBaseElement > setSuperClass(PreactBaseElement, AMP.BaseElement)
• Bento Hierarchy: BentoFitText > PreactBaseElement

cc @alanorozco @kvchari @jridgewell

[kvchari]

One thing I might have missed we last discussed was how to share logic between AmpFitText and BentoFitText related to "fit-text" functionality since their class hierarchies only share Preact logic now. More generally, there is logic that both Amp and Bento version of each extension need. For example:

• amp/bento accordions shim slotted accordion panel headers and bodies as well as update the expanded panel based on the expanded attribute in the BaseElement class for the accordion extension.
• amp/bento base carousels trigger slide changes when the slide attribute changes
• amp/bento inline galleries wrap their child elements in a preact context
• ...

There are at least 10 extensions that have "non-trivial" BaseElements:

• amp-accordion
• amp-base-carousel
• amp-inline-gallery
• amp-lightbox
• amp-lightbox-gallery
• amp-selector
• amp-sidebar
• amp-soundcloud
• amp-stream-gallery
• amp-timeago

What was the solution to this? (other than obviously copy/pasting this logic into both AmpAccordion and BentoAccordion)

[samouri]

Could we do the same trick? AmpFitText extends setSuperclass(BentoFitText, AmpPreactBaseElement)

[kvchari]

Yeah I like that

An alternative could be

class FitTextBaseElement extends PreactBaseElement {}
class BentoFitText extends FitTextBaseElement {}
class AmpPreactBaseElement extends setSuperclass(PreactBaseElement, AMP.BaseElement) {} 
class AmpFitText extends setSuperclass(FitTextBaseElement, AmpPreactBaseElement) {}

to allow the BentoFitText component to have non-amp-compliant code. But that should be relatively easy to change after the fact, so I'll go with your solution.

[alanorozco]

I was thinking that to achieve something like:

class BentoFitText extends setSuperClass(PreactBaseElement, HTMLElement) {}

class AmpPreactBaseElement extends setSuperclass(PreactBaseElement, AMP.BaseElement) {} 
class AmpFitText extends AmpPreactBaseElement {}

....we wouldn't need a hierarchy to share behavior between AMP and Bento versions.

Bear in mind that we'd either duplicate ['props'], or import an independently defined object:

import {props} from './props';
BaseElement['props'] = props;

import {props} from './props';
AmpFoo['props'] = props;

Regarding the extensions that @kvchari mentioned:

Accordion

• Only shares an init() implementation that only accesses public methods. It can be refactored to a shared function that both call on init().

Selector

• (Same as Accordion): Only shares an init() implementation that only accesses public methods. It can be refactored to a shared function that both call on init().

BaseCarousel

• defaultSlide should be listed in ['props'] instead. It should not keep the value of slide_, it should let goToSlide() compare state from within the Preact component, indiscriminately passing the attribute's value. This is short enough that it's ok to duplicate.

• The contents of onSlideChange can be refactored to triggerSlideChangeEvent(this, index).

StreamGallery

• (Like part of BaseCarousel): The contents of onSlideChange can be refactored to triggerSlideChangeEvent(this, index).

InlineGallery

• Shared complexity is already contained in <ContextExporter />, which we can import independently.

Lightbox

• Currently, AMP overrides almost everything so it's already independent from Bento. Only shared method is mutationObserverCallback(). It's a toggle whose state the Preact component should manage instead. Refactor into this.api().toggle() and it should be ok to duplicate.

Sidebar

• (Same as Lightbox): Currently, AMP overrides almost everything so it's already independent from Bento. Only shared method is mutationObserverCallback(). It's a toggle whose state the Preact component should manage instead. Refactor into this.api().toggle() and it should be ok to duplicate.

Soundcloud

• getPreconnects() should be in AMP impl but otherwise I don't see anything shared?

Timeago

• I don't see anything non-trivial shared?

---

And a bonus:

Video

• All video components extend from VideoBaseElement. This only calls registerApiAction, which is only necessary on the AMP layer can call a helper function on init() instead. VideoBaseElement can then be removed.

[kvchari]

Currently implementing the changes discussed here. Draft PR is available to see the idea: #37156 (although will likely be closed in favor or multiple smaller PRs).

One point related to these changes is that bento components were being implicitly tested by the tests for the amp component in the test-amp-*.js files. By splitting up the hierarchy st amp and bento components do not share a common base class (beyond PBE), we lose this implicit testing. to remediate this, I also need to split up the existing test-amp-*.js files into bento-related and amp-related tests. This will take extra time but would have eventually been necessary anyway because of ongoing discussions to create a separate repo for bento components.

[samouri]

updates from today's adhoc meeting
tldr: The AMP Hierarchy gets more confusing but the Bento one becomes simple.

class BentoFitText extends PreactBaseElement {}
class AmpPreactBaseElement extends setSuperclass(PreactBaseElement, AMP.BaseElement) {} 
class AmpFitText extends setSuperclass(BentoFitText, AmpPreactBaseElement) {}

[kvchari]

The video embed components add an extra layer of complexity here:

Using amp-brightcove as an example, the existing hierarchy is:
AmpBrightcove > BentoBrightcove > AmpVideoBase > BentoVideo > PreactBaseElement > AMP.BaseElement
BentoBrightcove > AmpVideoBase > BentoVideo > PreactBaseElement > AMP.BaseElement

Note

• BentoBrightcove/BentoVideo is the BaseElement class defined in the base-element.js file for the amp-brightcove/amp-video extensions respectively
• AmpVideoBase is VideoBaseElement from extensions/amp-video/video-base-element.js

The issue is that all the Amp* video components extend from AmpVideoBase (which adds extra amp-specific, video-specific functionality) so something like:
class AmpBrightcove extends setSuperClass(BentoBrightcove, AmpPreactBaseElement) (like described above)
will drop that logic. I think we can get around this for video components like this:

// for amp layer
class AmpPreactBaseElement extends setSuperClass(PreactBaseElement, AMP.BaseElement) {}
class AmpVideoBaseElement extends setSuperClass(BentoVideo, AmpPreactBaseElement) {}
class AmpBrightcove extends setSuperClass(BentoBrightcove, AmpVideoBaseElement) {}

// for bento layer
class BentoVideo extends PreactBaseElement {}
class BentoBrightcove extends BentoVideo {}

[samouri]

@kvchari: makes sense to me! Ridiculous number of layers on the AMP side of things, but I don't see a way to avoid it. So it goes.

[samouri]

Closing as the hierarchy has been solved. Remaining clarity improvements are being tracked by #37538