-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Introduce AMP Live List Cache Busting #25295
Changes from 17 commits
cfc797f
7c9902e
e1eb02c
3a5465a
870b22f
e81a2d1
e073300
03935f4
1c78f0d
ce266db
2073a5b
78062ac
d0feed7
2372cbf
c237398
5af1ea2
99127ec
3b689d8
f948724
166e182
2ac4168
30ef565
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,8 @@ | |
|
||
import {Poller} from './poller'; | ||
import {Services} from '../../../src/services'; | ||
import {addParamToUrl} from '../../../src/url'; | ||
import {addParamsToUrl} from '../../../src/url'; | ||
import {dict} from '../../../src/utils/object'; | ||
import {fetchDocument} from '../../../src/document-fetcher'; | ||
import {getMode} from '../../../src/mode'; | ||
import {getServicePromiseForDoc} from '../../../src/service'; | ||
|
@@ -29,6 +30,10 @@ export const SERVICE_ID = 'liveListManager'; | |
|
||
const TRANSFORMED_PREFIX = 'google;v='; | ||
|
||
// Maxiumum random number used as a query parameter. | ||
// Cannot use Number.MAX_SAFE_INTEGER due to IE Compatibility. | ||
const AMP_LIVE_LIST_MAX_RANDOM_NUMBER = 9007199254740991; | ||
|
||
/** | ||
* Property used for storing id of custom slot. This custom slot can be used to | ||
* replace the default "items" and "update" slot. | ||
|
@@ -45,8 +50,9 @@ export const AMP_LIVE_LIST_CUSTOM_SLOT_ID = 'AMP_LIVE_LIST_CUSTOM_SLOT_ID'; | |
export class LiveListManager { | ||
/** | ||
* @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc | ||
* @param {boolean} enrolledInAppendRandomExperiment | ||
*/ | ||
constructor(ampdoc) { | ||
constructor(ampdoc, enrolledInAppendRandomExperiment) { | ||
/** @const */ | ||
this.ampdoc = ampdoc; | ||
|
||
|
@@ -77,6 +83,9 @@ export class LiveListManager { | |
/** @private @const {boolean} */ | ||
this.isTransformed_ = isDocTransformed(ampdoc.getRootNode()); | ||
|
||
/** @private {boolean} */ | ||
this.enrolledInAppendRandomExperiment_ = enrolledInAppendRandomExperiment; | ||
|
||
// Only start polling when doc is ready and when the doc is visible. | ||
this.whenDocReady_().then(() => { | ||
// Switch out the poller interval if we can find a lower one and | ||
|
@@ -147,11 +156,19 @@ export class LiveListManager { | |
fetchDocument_() { | ||
let url = this.url_; | ||
if (this.latestUpdateTime_ > 0) { | ||
url = addParamToUrl( | ||
url, | ||
'amp_latest_update_time', | ||
String(this.latestUpdateTime_) | ||
); | ||
const parameters = this.enrolledInAppendRandomExperiment_ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there's a race condition here but the poll interval probably masks it. Probably worth testing to see. |
||
? dict({ | ||
'amp_latest_update_time': String(this.latestUpdateTime_), | ||
// AMP Caches do not always evict entries from their caches. | ||
// This experiment adds a random identifier to reduce cache hits for enrolled documents. | ||
'amp_random': String( | ||
Math.floor(Math.random() * AMP_LIVE_LIST_MAX_RANDOM_NUMBER) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we decide to not go with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Given the timing for the fix, and the remedy on the Google AMP Cache – I instead opted for the more narrow fix for enrolled documents.
kristoferbaxter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
), | ||
}) | ||
: dict({ | ||
'amp_latest_update_time': String(this.latestUpdateTime_), | ||
}); | ||
url = addParamsToUrl(url, parameters); | ||
} | ||
|
||
if (this.isTransformed_) { | ||
|
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.
Will
enrolled
propagate to theLiveListManager
constructor?amphtml/extensions/amp-live-list/0.1/live-list-manager.js
Lines 123 to 128 in f49ba4e
Maybe we should do this check in
LiveListManager
on the firstregister()
call instead. You can get an element fromliveList.element
in that function.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.
Can refactor this way, will be a little bit.
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.
Moved over, mind taking another look?