Add AMP loader to <amp-facebook-*> #14545
Conversation
|
@cvializ could you take a look? Thanks! |
| if (this.iframe_ && event.source != this.iframe_.contentWindow) { | ||
| return; | ||
| } | ||
| if (!getData(event) || !(isObject(getData(event)) |
There was a problem hiding this comment.
nit: I would cache the value of getData(event) in a local variable so you don't have to keep calling getData.
| } | ||
|
|
||
| /** @const {?JsonObject} */ | ||
| const eventData = /** @type {?JsonObject} */ (isObject(getData(event)) |
There was a problem hiding this comment.
When would event.data be a JSON string and when would it be a JSON object? Does the Facebook API return both?
There was a problem hiding this comment.
No this is because the iframe is also sending some ill formed JSON messages which I need to gracefully handle as well. Had to do the same for amp-bodymovin-animation.js - https://github.com/ampproject/amphtml/blob/master/extensions/amp-bodymovin-animation/0.1/amp-bodymovin-animation.js#L145.
| return; | ||
| } | ||
| if (!getData(event) || !(isObject(getData(event)) | ||
| || startsWith(/** @type {string} */ (getData(event)), '{'))) { |
There was a problem hiding this comment.
Attempting to parseJson might be a better check of whether it is valid JSON rather than checking the first character. What do you think?
There was a problem hiding this comment.
Yes. Definitely. Didn't know a function like tryParseJSon existed. Changing the code to use that instead.
| } | ||
|
|
||
| /** @const {?JsonObject} */ | ||
| const eventData = /** @type {?JsonObject} */ (isObject(getData(event)) |
There was a problem hiding this comment.
No this is because the iframe is also sending some ill formed JSON messages which I need to gracefully handle as well. Had to do the same for amp-bodymovin-animation.js - https://github.com/ampproject/amphtml/blob/master/extensions/amp-bodymovin-animation/0.1/amp-bodymovin-animation.js#L145.
| return; | ||
| } | ||
| if (!getData(event) || !(isObject(getData(event)) | ||
| || startsWith(/** @type {string} */ (getData(event)), '{'))) { |
There was a problem hiding this comment.
Yes. Definitely. Didn't know a function like tryParseJSon existed. Changing the code to use that instead.
| if (this.iframe_ && event.source != this.iframe_.contentWindow) { | ||
| return; | ||
| } | ||
| if (!getData(event) || !(isObject(getData(event)) |
| return; | ||
| } | ||
| let eventData = getData(event); | ||
| if (!eventData || !(isObject(eventData) || tryParseJson(eventData))) { |
There was a problem hiding this comment.
I'm still having a little trouble quickly reading these if statements. Would something like this be equivalent?
const eventData = getData(event);
if (!eventData) {
return;
}
const parsedEventData = isObject(eventData) ? eventData : tryParseJson(eventData);
if (!parsedEventData) {
return;
}There was a problem hiding this comment.
Made the changes you asked for. Thanks for the suggestion!
There was a problem hiding this comment.
You're welcome! And if you ever feel like my suggestions are too nit-picky feel free to say you'll keep it how you prefer.
|
Consider adding a test for this feature as a follow up. I see a test for resize messages in |
* Add loader to <amp-facebook-*> * Travis checks * Undo unintentional changes * Refactor code to reduce function calls. Use tryParseJson * Travis fixes * refaactor handleFacebookMessages * travis fixes
This fixes #13573
We send a message from the
3p.jsfile over to the <amp-facebook-*>.js API to disable the loader when the SDK is loaded and ready to paint.