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.

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

Deferred build API and builder #32568

Merged
merged 18 commits into from
Feb 25, 2021
1 change: 1 addition & 0 deletions build-system/global-configs/experiments-const.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"BENTO_AUTO_UPGRADE": false,
"INI_LOAD_INOB": false,
"V1_IMG_DEFERRED_BUILD": false,
"WITHIN_VIEWPORT_INOB": false
}
14 changes: 14 additions & 0 deletions build-system/tasks/presubmit-checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,18 @@ const forbiddenTerms = {
'extensions/amp-analytics/0.1/requests.js',
],
},
'\\.buildInternal': {
message: 'can only be called by the framework',
allowlist: [
'src/service/builder.js',
'src/service/resource.js',
'testing/iframe.js',
],
},
'getBuilderForDoc': {
message: 'can only be used by the runtime',
allowlist: ['src/custom-element.js', 'src/service/builder.js'],
},
// Service factories that should only be installed once.
'installActionServiceForDoc': {
message: privateServiceFactory,
Expand Down Expand Up @@ -308,6 +320,7 @@ const forbiddenTerms = {
'src/chunk.js',
'src/element-service.js',
'src/service.js',
'src/service/builder.js',
'src/service/cid-impl.js',
'src/service/origin-experiments-impl.js',
'src/services.js',
Expand Down Expand Up @@ -1384,6 +1397,7 @@ function hasAnyTerms(srcFile) {
/^test-/.test(basename) ||
/^_init_tests/.test(basename) ||
/_test\.js$/.test(basename) ||
/testing\//.test(srcFile) ||
/storybook\/[^/]+\.js$/.test(srcFile);
if (!isTestFile) {
hasSrcInclusiveTerms = matchTerms(srcFile, forbiddenTermsSrcInclusive);
Expand Down
89 changes: 78 additions & 11 deletions builtins/amp-img.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import {BaseElement} from '../src/base-element';
import {Layout, isLayoutSizeDefined} from '../src/layout';
import {ReadyState} from '../src/ready-state';
import {Services} from '../src/services';
import {dev} from '../src/log';
import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../src/utils/img';
Expand Down Expand Up @@ -45,11 +46,39 @@ const ATTRIBUTES_TO_PROPAGATE = [
];

export class AmpImg extends BaseElement {
/** @override @nocollapse */
static V1() {
return V1_IMG_DEFERRED_BUILD;
}

/** @override @nocollapse */
static prerenderAllowed() {
return true;
}

/** @override @nocollapse */
static getPreconnects(element) {
const src = element.getAttribute('src');
if (src) {
return [src];
}

// NOTE(@wassgha): since parseSrcset is computationally expensive and can
// not be inside the `buildCallback`, we went with preconnecting to the
// `src` url if it exists or the first srcset url.
const srcset = element.getAttribute('srcset');
if (srcset) {
// We try to find the first url in the srcset
const srcseturl = /\S+/.exec(srcset);
// Connect to the first url if it exists
if (srcseturl) {
return [srcseturl[0]];
}
}

return null;
}

/** @param {!AmpElement} element */
constructor(element) {
super(element);
Expand Down Expand Up @@ -106,6 +135,10 @@ export class AmpImg extends BaseElement {
if (!IS_ESM) {
guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_);
}

if (AmpImg.V1() && !this.img_.complete) {
this.setReadyState(ReadyState.LOADING);
}
}
}

Expand Down Expand Up @@ -260,6 +293,38 @@ export class AmpImg extends BaseElement {
return false;
}

/** @override */
buildCallback() {
if (!AmpImg.V1()) {
return;
}

// A V1 amp-img loads and reloads automatically.
this.setReadyState(ReadyState.LOADING);
this.initialize_();
const img = dev().assertElement(this.img_);
if (img.complete) {
this.setReadyState(ReadyState.COMPLETE);
this.firstLayoutCompleted();
this.hideFallbackImg_();
}
listen(img, 'load', () => {
this.setReadyState(ReadyState.COMPLETE);
this.firstLayoutCompleted();
this.hideFallbackImg_();
});
listen(img, 'error', (reason) => {
this.setReadyState(ReadyState.ERROR, reason);
this.onImgLoadingError_();
});
}

/** @override */
ensureLoaded() {
const img = dev().assertElement(this.img_);
img.loading = 'eager';
}

/** @override */
layoutCallback() {
this.initialize_();
Expand All @@ -275,6 +340,12 @@ export class AmpImg extends BaseElement {

/** @override */
unlayoutCallback() {
if (AmpImg.V1()) {
// TODO(#31915): Reconsider if this is still desired for V1. This helps
// with network interruption when a document is inactivated.
return;
}

if (this.unlistenError_) {
this.unlistenError_();
this.unlistenError_ = null;
Expand Down Expand Up @@ -319,10 +390,8 @@ export class AmpImg extends BaseElement {
!this.allowImgLoadFallback_ &&
this.img_.classList.contains('i-amphtml-ghost')
) {
this.getVsync().mutate(() => {
this.img_.classList.remove('i-amphtml-ghost');
this.toggleFallback(false);
});
this.img_.classList.remove('i-amphtml-ghost');
this.toggleFallback(false);
}
}

Expand All @@ -332,13 +401,11 @@ export class AmpImg extends BaseElement {
*/
onImgLoadingError_() {
if (this.allowImgLoadFallback_) {
this.getVsync().mutate(() => {
this.img_.classList.add('i-amphtml-ghost');
this.toggleFallback(true);
// Hide placeholders, as browsers that don't support webp
// Would show the placeholder underneath a transparent fallback
this.togglePlaceholder(false);
});
this.img_.classList.add('i-amphtml-ghost');
this.toggleFallback(true);
// Hide placeholders, as browsers that don't support webp
// Would show the placeholder underneath a transparent fallback
this.togglePlaceholder(false);
this.allowImgLoadFallback_ = false;
}
}
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-auto-ads/0.1/placement.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ export class Placement {
return (
whenUpgradedToCustomElement(this.getAdElement())
// Responsive ads set their own size when built.
.then(() => this.getAdElement().whenBuilt())
.then(() => this.getAdElement().build())
.then(() => {
const resized = !this.getAdElement().classList.contains(
'i-amphtml-layout-awaiting-size'
Expand All @@ -231,7 +231,7 @@ export class Placement {
// synchronously. So we explicitly wait for CustomElement to be
// ready.
return whenUpgradedToCustomElement(this.getAdElement())
.then(() => this.getAdElement().whenBuilt())
.then(() => this.getAdElement().build())
.then(() => {
return this.mutator_.requestChangeSize(
this.getAdElement(),
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-consent/0.1/consent-ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ export class ConsentUI {
// at build time. (see #18841).
if (isAmpElement(this.ui_)) {
whenUpgradedToCustomElement(this.ui_)
.then(() => this.ui_.whenBuilt())
.then(() => this.ui_.build())
.then(() => show());
} else {
show();
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-form/0.1/amp-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ export class AmpForm {
EXTERNAL_DEPS.join(',')
);
// Wait for an element to be built to make sure it is ready.
const promises = toArray(depElements).map((el) => el.whenBuilt());
const promises = toArray(depElements).map((el) => el.build());
return (this.dependenciesPromise_ = this.waitOnPromisesOrTimeout_(
promises,
2000
Expand Down
114 changes: 55 additions & 59 deletions extensions/amp-form/0.1/test/test-amp-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

import '../../../amp-mustache/0.1/amp-mustache';
import '../../../amp-selector/0.1/amp-selector';
import * as xhrUtils from '../../../../src/utils/xhr-utils';
import {ActionService} from '../../../../src/service/action-impl';
import {ActionTrust} from '../../../../src/action-constants';
Expand All @@ -25,6 +24,7 @@ import {
AmpFormService,
checkUserValidityAfterInteraction_,
} from '../amp-form';
import {AmpSelector} from '../../../amp-selector/0.1/amp-selector';
import {
AsyncInputAttributes,
AsyncInputClasses,
Expand Down Expand Up @@ -2287,74 +2287,70 @@ describes.repeated(
});
});

it('should submit after timeout of waiting for amp-selector', function () {
it('should submit after timeout of waiting for amp-selector', async function () {
expectAsyncConsoleError(/Form submission failed/);
this.timeout(3000);
return getAmpForm(getForm()).then((ampForm) => {
const form = ampForm.form_;
const selector = createElement('amp-selector');
selector.setAttribute('name', 'color');
form.appendChild(selector);

env.sandbox
.stub(selector, 'whenBuilt')
.returns(new Promise((unusedResolve) => {}));
env.sandbox.spy(ampForm, 'handleSubmitAction_');
const ampForm = await getAmpForm(getForm());
const form = ampForm.form_;
const selector = createElement('amp-selector');
selector.setAttribute('name', 'color');

const submitPromise = ampForm.actionHandler_({
method: 'submit',
satisfiesTrust: () => true,
});
expect(ampForm.handleSubmitAction_).to.have.not.been.called;
return timer
.promise(1)
.then(() => {
expect(ampForm.handleSubmitAction_).to.have.not.been.called;
return timer.promise(2000);
})
.then(() => {
expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;
return submitPromise;
});
env.sandbox
.stub(AmpSelector.prototype, 'buildCallback')
.returns(new Promise((unusedResolve) => {}));
env.sandbox.spy(ampForm, 'handleSubmitAction_');

form.appendChild(selector);

const submitPromise = ampForm.actionHandler_({
method: 'submit',
satisfiesTrust: () => true,
});
expect(ampForm.handleSubmitAction_).to.have.not.been.called;

await timer.promise(1);
expect(ampForm.handleSubmitAction_).to.have.not.been.called;

await timer.promise(2000);
expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;

await submitPromise;
});

it('should wait for amp-selector to build before submitting', () => {
return getAmpForm(getForm()).then((ampForm) => {
let builtPromiseResolver_;
const form = ampForm.form_;
const selector = createElement('amp-selector');
selector.setAttribute('name', 'color');
form.appendChild(selector);
it('should wait for amp-selector to build before submitting', async () => {
const ampForm = await getAmpForm(getForm());

env.sandbox.stub(selector, 'whenBuilt').returns(
new Promise((resolve) => {
builtPromiseResolver_ = resolve;
})
);
env.sandbox.stub(ampForm.xhr_, 'fetch').resolves();
env.sandbox.spy(ampForm, 'handleSubmitAction_');
let builtPromiseResolver_;
const form = ampForm.form_;
const selector = createElement('amp-selector');
selector.setAttribute('name', 'color');

ampForm.actionHandler_({
method: 'submit',
satisfiesTrust: () => true,
});
expect(ampForm.handleSubmitAction_).to.have.not.been.called;
return timer
.promise(1)
.then(() => {
expect(ampForm.handleSubmitAction_).to.have.not.been.called;
return timer.promise(100);
})
.then(() => {
expect(ampForm.handleSubmitAction_).to.have.not.been.called;
builtPromiseResolver_();
return timer.promise(1);
})
.then(() => {
expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;
});
env.sandbox.stub(AmpSelector.prototype, 'buildCallback').returns(
new Promise((resolve) => {
builtPromiseResolver_ = resolve;
})
);
env.sandbox.stub(ampForm.xhr_, 'fetch').resolves();
env.sandbox.spy(ampForm, 'handleSubmitAction_');

form.appendChild(selector);

ampForm.actionHandler_({
method: 'submit',
satisfiesTrust: () => true,
});
expect(ampForm.handleSubmitAction_).to.have.not.been.called;

await timer.promise(1);
expect(ampForm.handleSubmitAction_).to.have.not.been.called;

await timer.promise(100);
expect(ampForm.handleSubmitAction_).to.have.not.been.called;
builtPromiseResolver_();

await timer.promise(1);
expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;
});

describe('Var Substitution', () => {
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-sticky-ad/1.0/amp-sticky-ad.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class AmpStickyAd extends AMP.BaseElement {
dev().assertElement(this.ad_)
)
.then((ad) => {
return ad.whenBuilt();
return ad.build();
})
.then(() => {
return this.mutateElement(() => {
Expand Down Expand Up @@ -188,7 +188,7 @@ class AmpStickyAd extends AMP.BaseElement {
*/
scheduleLayoutForAd_() {
whenUpgradedToCustomElement(dev().assertElement(this.ad_)).then((ad) => {
ad.whenBuilt().then(this.layoutAd_.bind(this));
ad.build().then(() => this.layoutAd_());
});
}

Expand Down
Loading