amp-bind: Fix state/ready race condition #21033
Conversation
dreamofabear
force-pushed
the
bind-ready-race
branch
from
a2c55c0
to
6a7d157
Compare
|
/to @alabiaga |
extensions/amp-bind/0.1/amp-state.js
Outdated
| this.updateState_(json, isInit); | ||
| }); | ||
| // Don't fetch in prerender mode. | ||
| const viewer = Services.viewerForDoc(this.getAmpDoc()); |
There was a problem hiding this comment.
fetchAndUpdate_ is also called in the registered refresh action method call. We should set the viewer service as a private property on the buildCallback so that we don't need to get the service everything time this method is called.
There was a problem hiding this comment.
I'd prefer not to do this optimization due to the readability and testing cost. I don't think it makes a perf difference in practice.
| this.fetchAndUpdate_(/* isInit */ true); | ||
| } | ||
|
|
||
| this.registerAction('refresh', () => { |
There was a problem hiding this comment.
I should have made this change when I added refresh support for amp-state but we should only register the action if the element has a src attribute. We should move this block in the if statement above.
if (this.element.hasAttribute('src')) {
this.fetchAndUpdate_(/* isInit */ true);
this.registerAction('refresh', ...
}
There was a problem hiding this comment.
Good call. Though it's possible for the element to not have a src attribute at buildCallback but have one later (via amp-bind), so I added a userAssert here instead.
| this.fetchAndUpdate_(/* isInit */ true); | ||
| } | ||
|
|
||
| this.registerAction('refresh', () => { |
There was a problem hiding this comment.
Good call. Though it's possible for the element to not have a src attribute at buildCallback but have one later (via amp-bind), so I added a userAssert here instead.
extensions/amp-bind/0.1/amp-state.js
Outdated
| this.updateState_(json, isInit); | ||
| }); | ||
| // Don't fetch in prerender mode. | ||
| const viewer = Services.viewerForDoc(this.getAmpDoc()); |
There was a problem hiding this comment.
I'd prefer not to do this optimization due to the readability and testing cost. I don't think it makes a perf difference in practice.
* First pass on fixing race condition. * Fix amp-state tests. * Fix test-bind-impl.js. * Check for unbuilt <amp-state> instead of N setState() calls. * Tweak comments. * Add userAssert for 'src' attribute in refresh action.
* First pass on fixing race condition. * Fix amp-state tests. * Fix test-bind-impl.js. * Check for unbuilt <amp-state> instead of N setState() calls. * Tweak comments. * Add userAssert for 'src' attribute in refresh action.
In some cases,
bindReadyviewer message can be sent before all<amp-state>elements are initialized (local data parsed). Fix this by:<script>child) inbuildCallback()before viewer is visible.bindReadymessage until all<amp-state>are built.