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
fix latestUpdateTime not getting initialized for live list manager #4270
Conversation
281855a
to
7d32606
Compare
7d32606
to
ecbe320
Compare
@lannka PTAL. fixes a bug in live list where the first GET request does not contain the "amp_latest_update_time" parameter on its initial request |
0afb6e4
to
8a8647a
Compare
@@ -137,6 +140,17 @@ describe('LiveListManager', () => { | |||
}); | |||
}); | |||
|
|||
it('should get the amp_latest_update_time on doc ready', () => { |
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.
Not sure if it's feasible, but it would be better to test where this private value is used than where it is filled.
In this case, a more robust test would be to check whether the number is finally in the fetch request URL.
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.
done.
LGTM with one suggestion on the test. |
8a8647a
to
5f3d030
Compare
@lannka this fixes the live-blog sample in your PR. I need to fix the live-list sample next |
5f3d030
to
7ea975a
Compare
LGTM |
7ea975a
to
233a7a0
Compare
No description provided.