From 2f9531b5d9250575d01e3435821a719048129d54 Mon Sep 17 00:00:00 2001 From: William Chou Date: Mon, 25 Feb 2019 15:05:43 -0500 Subject: [PATCH 1/3] Simplify amp-state race fix. --- extensions/amp-bind/0.1/bind-impl.js | 27 +++++++------------ .../0.1/test/integration/test-bind-impl.js | 24 +++++++---------- 2 files changed, 18 insertions(+), 33 deletions(-) diff --git a/extensions/amp-bind/0.1/bind-impl.js b/extensions/amp-bind/0.1/bind-impl.js index 8128705fb9cf..a5aa51fa66c5 100644 --- a/extensions/amp-bind/0.1/bind-impl.js +++ b/extensions/amp-bind/0.1/bind-impl.js @@ -186,15 +186,11 @@ export class Bind { return this.initialize_(root); }); - /** @private {?Node} */ - this.rootNode_ = null; - /** @private {Promise} */ this.setStatePromise_ = null; /** @private @const {!../../../src/utils/signals.Signals} */ this.signals_ = new Signals(); - this.signals_.whenSignal('READY').then(() => this.onReady_()); // Install debug tools. const g = self.AMP; @@ -234,7 +230,6 @@ export class Bind { dev().info(TAG, 'state:', this.state_); if (opt_skipEval) { - this.checkReadiness_(); return Promise.resolve(); } @@ -454,8 +449,6 @@ export class Bind { * @private */ initialize_(root) { - this.rootNode_ = root; - // Disallow URL property bindings in AMP4EMAIL. const allowUrlProperties = !this.isAmp4Email_(); this.validator_ = new BindValidator(allowUrlProperties); @@ -475,25 +468,23 @@ export class Bind { if (getMode().development) { return this.evaluate_().then(results => this.verify_(results)); } - }).then(() => this.checkReadiness_()); + }).then(() => this.checkReadiness_(root)); } /** * Bind is "ready" when its initialization completes _and_ all * elements' local data is parsed and processed (not remote data). + * @param {!Node} root * @private */ - checkReadiness_() { - if (!this.rootNode_) { - return; - } - const unbuilt = - this.rootNode_.querySelectorAll('AMP-STATE.i-amphtml-notbuilt'); - if (unbuilt.length > 0) { - return; + checkReadiness_(root) { + const ampStates = root.querySelectorAll('AMP-STATE'); + if (ampStates.length > 0) { + const whenBuilt = toArray(ampStates).map(el => el.whenBuilt()); + Promise.all(whenBuilt).then(() => this.onReady_()); + } else { + this.onReady_(); } - // Use a signal to ensure that onReady_() is only invoked once. - this.initializePromise_.then(() => this.signals_.signal('READY')); } /** diff --git a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js index 57a15c0df393..4a2cedc3377a 100644 --- a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js +++ b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js @@ -308,24 +308,18 @@ describe.configure().ifChrome().run('Bind', function() { }); }); - it('should not send "bindReady" until all are built', () => { + it.only('should not send "bindReady" until all are built', () => { const element = createElement(env, container, '', 'amp-state', true); - element.classList.add('i-amphtml-notbuilt'); - - const signals = bind.signals(); - sandbox.spy(signals, 'signal'); - - expect(signals.signal).to.not.be.called; - expect(viewer.sendMessage).to.not.be.called; + let buildAmpState; + const builtPromise = new Promise(resolve => { + buildAmpState = resolve; + }); + element.whenBuilt = () => builtPromise; return onBindReady(env, bind).then(() => { - expect(signals.signal).to.not.be.called; - - // Simulate buildCallback(). - element.classList.remove('i-amphtml-notbuilt'); - bind.setState({}, /* opt_skipEval */ true); - - return signals.whenSignal('READY'); + expect(viewer.sendMessage).to.not.be.called; + buildAmpState(); + return element.whenBuilt(); }).then(() => { expect(viewer.sendMessage).to.be.calledOnce; expect(viewer.sendMessage).to.be.calledWith('bindReady'); From f3978fe8488c2aaaa49f566ff9e8ca6fc0a813e5 Mon Sep 17 00:00:00 2001 From: William Chou Date: Mon, 25 Feb 2019 15:06:11 -0500 Subject: [PATCH 2/3] Remove .only(). --- extensions/amp-bind/0.1/test/integration/test-bind-impl.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js index 4a2cedc3377a..51387b16ac24 100644 --- a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js +++ b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js @@ -308,7 +308,7 @@ describe.configure().ifChrome().run('Bind', function() { }); }); - it.only('should not send "bindReady" until all are built', () => { + it('should not send "bindReady" until all are built', () => { const element = createElement(env, container, '', 'amp-state', true); let buildAmpState; const builtPromise = new Promise(resolve => { From 74083a52e4915aac0980aa12d3c354db1520b429 Mon Sep 17 00:00:00 2001 From: William Chou Date: Mon, 25 Feb 2019 16:19:03 -0500 Subject: [PATCH 3/3] Fix other bind tests. --- .../amp-bind/0.1/test/integration/test-bind-impl.js | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js index 51387b16ac24..1a79a6dfc79e 100644 --- a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js +++ b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js @@ -293,16 +293,9 @@ describe.configure().ifChrome().run('Bind', function() { }); it('should send "bindReady" to viewer on init', () => { - const signals = bind.signals(); - sandbox.spy(signals, 'signal'); - - expect(signals.signal).to.not.be.called; expect(viewer.sendMessage).to.not.be.called; return onBindReady(env, bind).then(() => { - expect(signals.signal).to.be.calledWith('READY'); - return signals.whenSignal('READY'); - }).then(() => { expect(viewer.sendMessage).to.be.calledOnce; expect(viewer.sendMessage).to.be.calledWith('bindReady'); }); @@ -772,7 +765,9 @@ describe.configure().ifChrome().run('Bind', function() { it('should ignore updates if specified in setState()', () => { const element = createElement(env, container, '[src]="foo"', 'amp-state'); + element.whenBuilt = () => Promise.resolve(); expect(element.getAttribute('src')).to.be.null; + const promise = onBindReadyAndSetState(env, bind, {foo: '/foo'}, /* opt_isAmpStateMutation */ true); return promise.then(() => {