Provide a helper method waiting for HTMLElement upgrade to CustomElement by zhouyx · Pull Request #9542 · ampproject/amphtml

zhouyx · 2017-05-25T01:06:21Z

dvoytenko · 2017-05-26T20:09:59Z

src/dom.js

+ * Return a promise that resolve when an AMP element upgrade from HTMLElement
+ * to CustomElement
+ * @param {!Element} element
+ * @return {!Promise}


Let's do !Promise<!Element> for convenience.

dvoytenko · 2017-05-26T20:10:27Z

src/dom.js

+ * @param {!Element} element
+ * @return {!Promise}
+ */
+export function whenUpgradeToCustomElement(element) {


"whenUpgrade_d_ToCustomElement"

changed name

dvoytenko · 2017-05-26T20:11:53Z

src/dom.js

+    return Promise.resolve();
+  }
+  // If Element is still HTMLElement, wait for it to upgrade to customElement
+  if (!element.upgradeToCustomElementPromise_) {


This won't work unfortunately like this. Obfuscation will break between versions. Probably will have to work with "pure" strings.

Changed to element['upgradedToCustomElementPromise']. But I still don't get why element.upgradeToCustomElementPromise_ will break. I tested locally and it works fine before 😕

Due to obfuscation.

Thanks @dvoytenko for the explanation!! I would never be able to find out myself 😄

dvoytenko · 2017-05-26T20:12:30Z

src/custom-element.js

      /** @private {?./layout-delay-meter.LayoutDelayMeter} */
      this.layoutDelayMeter_ = null;
+
+      if (this.whenUpgradeToCustomElement_) {


See below, re: strings instead of private properties.

changed to this['whenUpgradedToCustomElement'], if this is what you mean here.

Our current format is smth like __AMP_NAME_OF_FIELD. Let's keep to that format.

changed to __AMP_UPGRADE_CUS_ELE_PROMISE__ like what we did in action-imp.js

Sorry to bikeshed. Let's just call __AMP_UPG_PRM and __AMP_UPG_RES.

Haha. You name it!

jridgewell · 2017-05-26T22:05:58Z

extensions/amp-sticky-ad/1.0/amp-sticky-ad.js

+      if (!ad.whenBuilt) {
+        // TODO(@zhouyx, #9126): Cleanup once make sure the fix works
+        dev().error(TAG, 'element whenBuilt still do not exist!');
+        timerFor(this.win).delay(() => {


This logic can be flattened:

whenUpgradedToCustomElement().then(ad => if (ad.whenBuilt) return ad; } // TODO return timerFor(this.win).promise(1000).then(() => ad); }).then(ad => { return ad.whenBuilt(); }).then(() => { this.mutateElement(() => toggle(this.element, true)); });

I prefer the if/else loop since I am pretty sure it won't get into the if loop. Just add it for extra check, will remove once confirm the fix works.

Why do we if (ad.whenBuilt)? We already await upgrade, so it's not needed. Right?

But the duplication.... 😢

haha, I think @dvoytenko is right. it's not needed. I already tested it and am pretty sure it fix the race condition bug in #9126. @jridgewell I deleted the duplication code 😄

jridgewell · 2017-05-26T22:09:22Z

src/dom.js

+  // may not be attached yet.
+  return startsWith(tag, 'AMP-') &&
+      // Some "amp-*" elements are not really AMP elements. :smh:
+      !(tag == 'AMP-STICKY-AD-TOP-PADDING' || tag == 'AMP-BODY');


Merge conflict? I thought this was already in there?

You didn't merge that PR 😈

jridgewell · 2017-05-26T22:10:31Z

src/dom.js

+ */
+export function whenUpgradedToCustomElement(element) {
+  dev().assert(isAmpElement(element), 'element is not AmpElement');
+  if (element.createdCallback) {


Use __AMP_RESOURCE, like Resources does.

Does a customElement guaranteed to have a resource at before createdCallback being called?

Not guaranteed. Adding it to DOM can happen much much later. So, I think, createdCallback is a good idea.

jridgewell · 2017-05-26T22:11:27Z

src/custom-element.js

+
+      if (this['whenUpgradedToCustomElement']) {
+        this['whenUpgradedToCustomElement'](this);
+        this['whenUpgradedToCustomElement'] = null;


Unset the promise, too.

what promise? element['upgradedToCustomElementPromise']? I am returning it?

Agree. Probably better to clear the promise out.

set element['promise'] to null. To confirm when I do return element['promise'] it is not returning a reference to the promise? So setting element['promise'] = null later doesn't affect the returned promise from #whenUpgradedToCustomElement function.

Shouldn't go to null, but rather delete this['ABC'].

jridgewell · 2017-05-26T23:50:19Z

extensions/amp-sticky-ad/1.0/amp-sticky-ad.js

-          this.mutateElement(() => {
-            toggle(this.element, true);
-          });
+      ad.whenBuilt().then(() => {


We can unnest:

then(() => { return ad.whenBuilt(); }).then(() => { this.mutateElement(() => toggle(this.element, true)); });

sorry I didn't get it, but what's the need to unnest here?

It's JS style when using Promises (we needed promises because of callback hell). When using promises, instead of chaining a bunch of #then calls inside a then block, you return the promise instead.

then(() => { return ad.whenBuilt().then(() => { // this is callback hell }); }); //vs then(() => { return ad.whenBuilt(); }).then(() => { // left indent is the same, so callback hell is avoided. });

I see, changed! Thanks for the explanation! However I don't think we have a style requirement for this, and I can find the second usage in a lot of places😢

wait for element to upgrade

36097f2

zhouyx requested a review from dvoytenko May 25, 2017 01:06

googlebot added the cla: yes label May 25, 2017

dvoytenko suggested changes May 26, 2017

View reviewed changes

use string

efe8925

jridgewell reviewed May 26, 2017

View reviewed changes

address comment

aeeb012

jridgewell approved these changes May 26, 2017

View reviewed changes

address comment

65b1bd7

dvoytenko approved these changes May 30, 2017

View reviewed changes

zhouyx merged commit e27a195 into ampproject:master May 30, 2017

zhouyx deleted the wait-for-customElement branch May 30, 2017 22:12

zhouyx mentioned this pull request Jun 5, 2017

Investigate race condition on custom-element stubbing #9126

Closed

ampprojectbot added the PR use: In Beta / Experimental label Jun 7, 2017

Conversation

zhouyx commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

zhouyx May 26, 2017 •

edited

Loading

zhouyx May 26, 2017 •

edited

Loading