New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
♻️ Move RTC manager to a service #32728
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to services.js
look fine to be, but could you loop in another reviewer who's more familiar with the rest of this code before I sign off? I don't feel confident reviewing this fully.
@rcebulko sorry, I forgot to add monetization reviewers. |
return this.getBlockRtc_().then((shouldBlock) => | ||
shouldBlock | ||
? undefined | ||
: Services.realTimeConfigForDoc( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we leave this wrapped in a try/catch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to be:
if (!! AMP.RealTimeConfigManager) {
try {
return new AMP.RealTimeConfigManager(
...
The try/catch, at that time, would prevent rtc issues from breaking the entire runtime. Now that everything is wrapped in the promise and ran asynchronously, the consequence of not using try/catch is much smaller. Throwing exceptions should not be the expected thing anyways, so I feel keeping try/catch nowadays is unnecessarily defensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but perhaps we should consider wrapping this in an experiment if possible, since we already broke it once.
@calebcordry I would like to, but RTV slots are full now. We can still try looking at the beta numbers (last time I didn't think it helped though). |
Redo #32386
#32713 Ran into this issue, and this PR has it fixed. The root cause is that different amp-ad elements are incorrectly sharing the same service instance.