Skip to content
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

A4A: Remove CSI pings from DOM #9117

Merged
merged 3 commits into from
May 4, 2017
Merged

Conversation

tdrl
Copy link

@tdrl tdrl commented May 3, 2017

Avoids writing CSI ping beacons to DOM -- now sends them just with new Image(). Maintains dev().info for console messaging in dev mode, though.

Copy link
Contributor

@keithwrightbos keithwrightbos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to consider is handling network failure. Take a look at reportingBackoff:
https://github.com/ampproject/amphtml/blob/master/src/error.js#L61

@tdrl
Copy link
Author

tdrl commented May 3, 2017

@keithwrightbos Sure... Seems like a good idea if we wanted to make this service more robust. But, really, do we? I mean, the plan is to deprecate it in favor of amp-analytics anyway. And I don't get a sense that we're missing so many pings that this is a big issue, do you?

1 similar comment
@tdrl
Copy link
Author

tdrl commented May 3, 2017

@keithwrightbos Sure... Seems like a good idea if we wanted to make this service more robust. But, really, do we? I mean, the plan is to deprecate it in favor of amp-analytics anyway. And I don't get a sense that we're missing so many pings that this is a big issue, do you?

@keithwrightbos
Copy link
Contributor

SGTM

@tdrl tdrl merged commit 3ff0dd7 into ampproject:master May 4, 2017
@tdrl tdrl deleted the a4a_no_dom_pings branch May 4, 2017 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants