Wait for amp-selector to build before submitting#7190
Wait for amp-selector to build before submitting#7190mkhatib merged 6 commits intoampproject:masterfrom
Conversation
extensions/amp-form/0.1/amp-form.js
Outdated
|
|
||
| /** | ||
| * A list of external dependencies that can be included in forms. | ||
| * @type {Array<string>} |
extensions/amp-form/0.1/amp-form.js
Outdated
| // Wait for an element to be built to make sure it is ready. | ||
| const depPromises = toArray(depElements).map(el => el.whenBuilt()); | ||
| return Promise.race( | ||
| [Promise.all(depPromises), this.timer_.promise(500)]); |
There was a problem hiding this comment.
500 is not enough. Let's go with at least 2s
src/custom-element.js
Outdated
| this.buildPromiseRejector_ = null; | ||
|
|
||
| /** @private {!Promise} */ | ||
| this.buildPromise_ = new Promise((resolve, reject) => { |
There was a problem hiding this comment.
You mean creating the promise? Yes sure.
|
@dvoytenko PTAL 👀 |
jridgewell
left a comment
There was a problem hiding this comment.
Do we need a new error state to explain why this form won't submit (when the selector failed to build)?
extensions/amp-form/0.1/amp-form.js
Outdated
| const depElements = this.form_.querySelectorAll(EXTERNAL_DEPS.join(',')); | ||
| // Wait for an element to be built to make sure it is ready. | ||
| const depPromises = toArray(depElements).map(el => el.whenBuilt()); | ||
| return Promise.race( |
There was a problem hiding this comment.
Can we cache this, so we don't have to re-query every time?
src/custom-element.js
Outdated
| if (this.buildPromise_) { | ||
| return this.buildPromise_; | ||
| } | ||
| return this.buildPromise_ = new Promise((resolve, reject) => { |
There was a problem hiding this comment.
There's no reason to hold onto these references is we're already built.
There was a problem hiding this comment.
This logic is now changed - @dvoytenko implemented a signal system in #7205
src/custom-element.js
Outdated
| return this.buildPromise_; | ||
| } | ||
| return this.buildPromise_ = new Promise((resolve, reject) => { | ||
| this.buildPromiseResolver_ = resolve; |
There was a problem hiding this comment.
We need to know whether the build failed as well.
766edbb to
666c470
Compare
|
Let's rename the PR. |
|
@dvoytenko & @jridgewell PTAL 👀 |
extensions/amp-form/0.1/amp-form.js
Outdated
| // Wait for an element to be built to make sure it is ready. | ||
| const depPromises = toArray(depElements).map(el => el.whenBuilt()); | ||
| return this.dependenciesPromise_ = Promise.race( | ||
| [Promise.all(depPromises), this.timer_.promise(15000)]); |
There was a problem hiding this comment.
Woops good catch I was testing this and forgot to reset it to 2000
extensions/amp-form/0.1/amp-form.js
Outdated
|
|
||
| /** | ||
| * Returns a promise that will be resolved when all dependencies used inside the form | ||
| * tag are loaded and built (e.g. amp-selector) or 500ms timeout - whichever is first. |
dvoytenko
left a comment
There was a problem hiding this comment.
LGTM with a tiny request for doc update.
| this.urlReplacement_ = urlReplacementsForDoc(element); | ||
|
|
||
| /** @private {?Promise} */ | ||
| this.dependenciesPromise_ = null; |
There was a problem hiding this comment.
nit(optional): i would call this - haveDepsLoadedPromise_
There was a problem hiding this comment.
Sticking to old name, the method name is more important as it describes the wait better.
Closes #7032
Closes #6951
Closes #6927