Deferred build API and builder

[dvoytenko]

Partial for #31915.

TODO:

• Support amp-video experiment for V2
• Clarify build scheduling: setTimeout vs queue vs etc
• Clarify preconnect behavior
• Remove debug info
• Add tests

[samouri]

This is really impressive! As much as I'm daunted by the scope of the change and its implication, it is 100% the right direction (deferring constructor, build, and layout). I haven't had a chance to review the test changes yet.

old (v1)
AMP Component authors write a class that extends BaseElement. It has two overridable functions: buildCallback and layoutCallback, both called by the API. buildCallback is essentially always called ASAP whereas layoutCallback is determined by a relatively complex set of rules around viewport intersection, displayability (size > 0), and relative priority to other components in the middle of being processed.

new (v2)
AMP Components author write a class that extends BaseElement. Only overridable fn of note is buildCallback. Instead of ASAP, this gets called by the Runtime when close to viewport. Most auxiliary callbacks are deprecated. Priority only determines whether setTimeout or setIdleCallback are used.

[dvoytenko]

@samouri responded to all comments. PTAL.

[jridgewell]

I'm assuming targets should already contain the target element?

[dvoytenko]

They likely do - this just overrides it for new urgency.

[jridgewell]

Can we add an assertion? I'm worried we'll hit a double build.

[dvoytenko]

The double-builds are definitely not possible. The CustomElement itself prevents them and there are a few tests on that subject. So it's ok even if somehow the build is scheduled twice. However, in this particular case it's very much expected that this call will override the existing information. It basically says: "don't worry about what you already know, just go ahead and build". Does it make sense?

[lgtm-com]

This pull request introduces 2 alerts when merging cc00b43 into fac0f5f - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable
• 1 for Assignment to constant

[samouri]

Only one potentially blocking comment regarding READY_SCAN_SIGNAL . The rest of my comments were either questions or nits.

[dvoytenko]

@samouri @jridgewell I've responded to all comments. PTAL.

[jridgewell]

Can we add an assertion? I'm worried we'll hit a double build.

[amp-owners-bot]

Hey @gmajoulet, @newmuis, @Enriqe! These files were changed:

extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/page-advancement.js

[dvoytenko]

Since we confirmed the API I switched a couple of relevant dependents to the new API. Where immediate build is needed, it now calls element.build() instead of whenBuilt(). These are just a few one-line changes. But I wanted them in the same PR in case a revert or a fix-forward needed.

[rsimha]

@dvoytenko I'm seeing several instances of a failing test since this PR was merged. Related?

  Rendering of amp-img
    ✗ should respect media queries
	AssertionError: expected null to exist
	    at _callee5$ (/tmp/test/integration/test-amp-img.js:94:7)
	    at tryCatch (/tmp/node_modules/fetch-mock/es5/client-bundle.js:1291:52)
	    at Generator.invoke [as _invoke] (/tmp/node_modules/fetch-mock/es5/client-bundle.js:1521:34)
	    at Generator.next (/tmp/node_modules/fetch-mock/es5/client-bundle.js:1346:33)
	    at asyncGeneratorStep (/tmp/e585455f262f4c5569ed473042a40762.browserify.js:148234:103)
	    at _next (/tmp/e585455f262f4c5569ed473042a40762.browserify.js:148236:194)

Example logs: https://app.circleci.com/pipelines/github/ampproject/amphtml/2856/workflows/1f12e9e9-0f86-4f6d-8120-5398e2f79dbb/jobs/33196/parallel-runs/0/steps/0-110

[samouri]

From the perspective of a BaseElement, the only effect of calling this.setReadyState is essentially the signaling, classList management, and loader toggling right?

[dvoytenko]

I'd say it's more than that. It includes:

• element.readyState value
• element.whenLoaded() promise. I guess it's what you mean by signaling. But it affects API state.
• element.onload and element.onerror events

And the things you mentioned:

• Loader
• classList, which I actually don't like and actively wondering if we should remove.

[samouri]

The reason I ask, is because I'm suspicious of the last two you said. Should a loading indicator be controlled by Runtime, or in the ideal world would it too be moved to the individual components

[dvoytenko]

It's controlled by each individual CustomElement. Are you wondering if it should be controlled by BaseElement?

[samouri]

By individual extensions of BaseElement. I.e. should amp-instagram control its own loading indicator.

[dvoytenko]

By individual extensions of BaseElement. I.e. should amp-instagram control its own loading indicator.

It does control it now individually. It's just provided on the CustomElement level which would always exist since it's part of CE API. The issue here is that loader is toggled even before the BaseElement is downloaded/upgraded in AMP.

But TBH I definitely see us moving more of the setReadyState into the BaseElement. Not yet sure if toggleLoading be part of it given the description above.