Wait for document ready before search for element in #getElement#9486
Wait for document ready before search for element in #getElement#9486zhouyx merged 5 commits intoampproject:masterfrom
Conversation
| // And no need to wait for document ready. | ||
| if (selector == ':root') { | ||
| return this.getRootElement(); | ||
| return Promise.resolve().then(() => { |
There was a problem hiding this comment.
Is the tick necessary here? Or can you just immediately resolve?
There was a problem hiding this comment.
No need. Just return Promise.resolve(...)
dvoytenko
left a comment
There was a problem hiding this comment.
Some notes. But overall I think this is a lot better than what it was before!
| * @param {?string=} selectionMethod Allowed values are `null`, | ||
| * `'closest'` and `'scope'`. | ||
| * @return {?Element} Element corresponding to the selector if found. | ||
| * @return {Promise<?Element>} Element corresponding to the selector if found. |
There was a problem hiding this comment.
It'll be {!Promise<?Element>}
| // And no need to wait for document ready. | ||
| if (selector == ':root') { | ||
| return this.getRootElement(); | ||
| return Promise.resolve().then(() => { |
There was a problem hiding this comment.
No need. Just return Promise.resolve(...)
| } | ||
| if (selector == ':host') { | ||
| return this.getHostElement(); | ||
| return Promise.resolve().then(() => { |
There was a problem hiding this comment.
Ditto, return Promise.resolve(this.getHostElement())
There was a problem hiding this comment.
I used promise.catch(error) in testing.
However when I use return Promise.resolve(user.assertElement(this.getHostElement())), I was not able to catch the error. Then I changed to Promise.resolve().then(() => return user.assertElement(this.getHostElement())) and then was able to catch the error. Can someone explain why this is the case? And how should I fix it?
There was a problem hiding this comment.
I see. Slightly weird. But I agree - it's probably better to yield error consistently.
There was a problem hiding this comment.
If the assertElement is throwing, then put it in a Promise constructor:
return new Promise(res => {
resolve(user().assertElement(this.getRootElement()));
});There was a problem hiding this comment.
changed as suggested : )
| * @param {?string=} selectionMethod Allowed values are `null`, | ||
| * `'closest'` and `'scope'`. | ||
| * @return {?AmpElement} AMP element corresponding to the selector if found. | ||
| * @return {Promise<?AmpElement>} AMP element corresponding to the selector if found. |
| } | ||
|
|
||
| return this.observers_[eventType].add(event => { | ||
| //console.log('add listener'); |
| //console.log('add listener'); | ||
| // Wait for target selected | ||
| targetReady.then(target => { | ||
| //console.log('trigger target handler'); |
| target = user().assertElement( | ||
| element, `Element "${selector}" not found`);; | ||
| return this.getElementSignal( | ||
| 'ini-load', /** @type{!Element} */ (element)); |
| selector, | ||
| selectionMethod | ||
| ).then(element => { | ||
| element = |
There was a problem hiding this comment.
Is it worth adding getRequiredAmpElement and getRequiredElement() APIs in the root? They seem to come up a lot. Or we can even modify the existing APIs if this is ALWAYS the case?
There was a problem hiding this comment.
I checked we always check if element is not null. Hence I moved to existing APIs. PTAL
6ebf9c8 to
ba9ea8f
Compare
fix #9370