✨ Add support for Mgid Ads in Web Stories#38588
✨ Add support for Mgid Ads in Web Stories#38588calebcordry merged 30 commits intoampproject:mainfrom
Conversation
|
|
erwinmombay
requested review from
calebcordry and
powerivq
and removed request for
erwinmombay
|
@powerivq @calebcordry could one of y'all take a look at this. thanks |
|
@powerivq @calebcordry Please, take a look on code. Also, I see there is a problem with unit-tests, but I'm not sure it's related to our changes. When I try to run them on local machine, I'm getting same errors even on main branch. |
|
@calebcordry Can you help me with unit tests? There are 6 tests for amp-strategy that are permanently failing and I don't understand how it's related to my changes. |
|
I restarted the tests, they are likely just flaky. |
|
I restarted the tests a few times and they keep failing even though they seem unrelated. Could you try merging or rebasing latest |
|
Done. Nothing changed. |
|
Yep I can repro the test failure on main, thanks for checking. I will try to fix it. |
|
Ok I have a pending PR to fix broken tests #38656. Ill try to get that merged Monday. Thanks for your patience on this. |
|
@velichkin ok the fixed was merged. Can you try to sync to latest main one more time please? |
|
@calebcordry Thanks, it's ok now. What's next? |
|
@dethstrobe can you take a look for the new folder |
| if (typeof networkInformation.saveData != 'undefined') { | ||
| params.push('nisd=' + (networkInformation.saveData ? 1 : 0)); | ||
| } | ||
| } catch (e) {} |
There was a problem hiding this comment.
Should this catch be...catching something? Or are we intentionally trying to obfuscate errors?
Do we even need a try/catch block here?
There was a problem hiding this comment.
It's intended. If in some weird browser networkInformation will be undefined, we should neither add params, neither trigger error.
| } | ||
| return Services.timerFor(this.win) | ||
| .timeoutPromise(timeoutInt, referrerPromise) | ||
| .catch(() => undefined); |
There was a problem hiding this comment.
Looks like another catch that is swallowing up errors. If this is intended, that's kind of odd, but we do a lot of odd things in AMP, so I guess it'll be fine. But if it's not intended then can we just remove the catch so that if there is an error, it just bubbles up like normal?
|
Woohoo, thanks for your patience on this one. |
|
Warning: disparity between this PR Percy build and its The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build /
|
7 participants
Adds support for the amp-ad type 'mgid' inside Web Stories. We already have integration with AMP through 3p iframe and it should work as it was before.