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

Extract AMPPreactBaseElement #37309

Merged
merged 23 commits into from
Jan 20, 2022
Merged

Extract AMPPreactBaseElement #37309

merged 23 commits into from
Jan 20, 2022

Conversation

kvchari
Copy link
Contributor

@kvchari kvchari commented Jan 6, 2022 •
edited
Loading

  • creates AmpPreactBaseElement
  • creates setSuperClass() to change the inheritance hierarchy of a class
  • ensures all "1.0" amp-* components inherit from AmpPreactBaseElement
  • moves (some) amp-specific methods into AmpPreactBaseElement
  • updates amp-* component docs/scaffolding schematics to refer to AmpPreactBaseElement
  • extracts CustomElement() static method out of CeBaseElement and into defineBentoElement() fn
    • ensures defineBentoElement() can work effectively in tests (where tests are run in an iframe)
  • renames VideoBaseElement -> AmpVideoBaseElement since it's purely amp-related
  • renames BaseElement in amp-video -> BentoVideoBaseElement to align

Partial #36950

samouri
samouri reviewed Jan 6, 2022
View reviewed changes
src/preact/amp-base-element.js
* @param {*} Y
* @return {*}
*/
export function setSuperClass(X, Y) {
Copy link
Member

Choose a reason for hiding this comment

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

Drive-by bikeshed (don't spend too much time here): I know @alanorozco and I disagree. But wanted to note here that I'm not a fan of the return X line. The code ends up looking cleaner, but also gives the appearance of purity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally get what you mean - returning X does give the illusion that a new class is created, when in reality X's prototype is being modified (which will affect all instances of X whether they realize or not)

I could change

class AmpFoo extends setSuperClass(BaseElement, AmpPreactBaseElement) {...}

to

setSuperClass(BaseElement, AmpPreactBaseElement)
class AmpFoo extends BaseElement {...}

This was referenced Jan 10, 2022
Closed
Closed
Closed
Closed
Closed
@kvchari kvchari force-pushed the extract-amp-pbe-final branch 5 times, most recently from b2aeac3 to ff7da91 Compare January 11, 2022 21:31
@kvchari kvchari marked this pull request as ready for review January 12, 2022 15:06
@amp-owners-bot
Copy link

Hey @westonruter, @ediamin! These files were changed:

extensions/amp-wordpress-embed/1.0/amp-wordpress-embed.js

@kvchari kvchari requested a review from samouri January 12, 2022 15:44
@samouri
Copy link
Member

samouri commented Jan 12, 2022

Partial for #36950

@kvchari kvchari force-pushed the extract-amp-pbe-final branch 4 times, most recently from 4e125db to 53aa9eb Compare January 12, 2022 16:12
@kvchari
Copy link
Contributor Author

kvchari commented Jan 12, 2022 •
edited
Loading

Outstanding work to address #36950

  • compress custom element wrapper + extension implementation into 1 class
    • currently each extension is still registered with a class inheriting from CeBaseElement which has an impl property that has an instance of the extension class (e.g. AmpAccordion, BentoAccordion, etc.). The wrapper+impl pattern was done to align with amp's class structure and isn't necessary for bento components.

Other outstanding quality of life refactors:

  • stop exporting define() functions for "sub components" of extensions (bento-facebook, bento-inline-gallery, bento-embedly-card all expose "sub coponents")
  • rename classes representing Bento components from BaseElement -> BentoFoo
  • rename files containing Bento components from base-element.js -> bento-foo.js
    • Note: currently autogenerated bento-foo.js files are used to publish standalone web component scripts, so there's a naming conflict here.
  • change testing infra to allow tests to run with AMP runtime to support <bento-*> unit testing
  • refactor setSuperClass such that it doesn't return so that it doesn't give the illusion of being a pure function

(addressed in separate PRs, tracked here)

samouri
samouri reviewed Jan 12, 2022
View reviewed changes
src/preact/amp-base-element.js Outdated
import {PreactBaseElement} from './base-element';

export class AmpPreactBaseElement extends PreactBaseElement {
// todo(kvchari): confirm this can be safely moved
Copy link
Member

@samouri samouri Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

flagging for presubmit visibility

samouri
samouri reviewed Jan 12, 2022
View reviewed changes
src/preact/base-element.js
* Displays loader. Override to customize.
* @protected
*/
handleOnLoad() {
Copy link
Member

Choose a reason for hiding this comment

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

FMI: Do bento WCs not support loading/fallback/placeholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that fallbacks and placeholders are an AMP concept and we're leaving the job of toggling an element on or off to consumers

@alanorozco can you confirm?

dmanek
dmanek approved these changes Jan 18, 2022
View reviewed changes
src/preact/amp-base-element.js Show resolved Hide resolved
estherkim
estherkim approved these changes Jan 19, 2022
View reviewed changes
Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

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

build system changes lgtm

@samouri samouri changed the title Extract amp pbe Extract AMPPreactBaseElement Jan 19, 2022
@samouri
Copy link
Member

samouri commented Jan 20, 2022

@kvchari: anything holding up this PR?

@kvchari kvchari merged commit 7c36fd2 into ampproject:main Jan 20, 2022
@kvchari kvchari deleted the extract-amp-pbe-final branch January 20, 2022 18:47
@samouri
Copy link
Member

samouri commented Jan 26, 2022

@kvchari: are you tracking the followup tasks anywhere?
I'd be happy to take a stab at one of them

@kvchari kvchari mentioned this pull request Feb 1, 2022
Closed
5 tasks
@kvchari
Copy link
Contributor Author

kvchari commented Feb 1, 2022

@samouri made an issue for them here: #37538

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samouri samouri samouri approved these changes

@estherkim estherkim estherkim approved these changes

@dethstrobe dethstrobe dethstrobe approved these changes

@dmanek dmanek dmanek approved these changes

@alanorozco alanorozco Awaiting requested review from alanorozco

@rbeckthomas rbeckthomas Awaiting requested review from rbeckthomas

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kvchari @samouri @dmanek @dethstrobe @estherkim @ampprojectbot