From 658c9133650e72a087b1ba184486db1fd9b8b047 Mon Sep 17 00:00:00 2001 From: Cathy Zhu Date: Fri, 7 Dec 2018 13:36:23 -0800 Subject: [PATCH 1/3] Fix race conditions --- extensions/amp-list/0.1/amp-list.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index 809965f77c65..cadc94172cc8 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -381,8 +381,8 @@ export class AmpList extends AMP.BaseElement { } return fetch.catch(error => { - if (opt_append && this.loadMoreFailedElement_) { - this.setLoadMoreFailed_(); + if (opt_append) { + throw error; } else { this.showFallback_(error); } @@ -797,6 +797,8 @@ export class AmpList extends AMP.BaseElement { this.unlistenLoadMore_(); this.unlistenLoadMore_ = null; } + }).catch(() => { + this.setLoadMoreFailed_(); }); } @@ -849,10 +851,10 @@ export class AmpList extends AMP.BaseElement { toggleLoadMoreLoading_(state) { this.mutateElement(() => { // If it's loading, then it's no longer failed or ended - if (this.loadMoreFailedElement_) { + if (this.loadMoreFailedElement_ && state) { this.loadMoreFailedElement_.classList.toggle('amp-visible', false); } - if (this.loadMoreEndElement_) { + if (this.loadMoreEndElement_ && state) { this.loadMoreEndElement_.classList.toggle('amp-visible', false); } if (this.loadMoreLoadingElement_) { @@ -880,9 +882,8 @@ export class AmpList extends AMP.BaseElement { this.unlistenLoadMore_ = listen(this.loadMoreFailedElement_, 'click', () => this.loadMoreCallback_()); } - if (this.loadMoreButton_) { - this.loadMoreButton_.classList.toggle('amp-visible', false); - } + this.loadMoreButton_.classList.toggle('amp-visible', false); + this.loadMoreLoadingElement_.classList.toggle('amp-visible', false); }); } From b83a9dd8a85376d61b619b69f38cdeec36b58b1a Mon Sep 17 00:00:00 2001 From: Cathy Zhu Date: Mon, 10 Dec 2018 14:19:01 -0800 Subject: [PATCH 2/3] Add endpoints and demo for failure --- build-system/app.js | 59 +++++- .../amp-list/infinite-scroll-fail.amp.html | 199 ++++++++++++++++++ 2 files changed, 247 insertions(+), 11 deletions(-) create mode 100644 test/manual/amp-list/infinite-scroll-fail.amp.html diff --git a/build-system/app.js b/build-system/app.js index fe401b19193b..e50619a0fe6d 100644 --- a/build-system/app.js +++ b/build-system/app.js @@ -1167,23 +1167,56 @@ app.get('/dist/ww(.max)?.js', (req, res) => { }); }); -app.get('/infinite-scroll', function(req, res) { - const {query} = req; - const items = []; - const numberOfItems = query['items'] || 10; - const pagesLeft = query['left'] || 1; - const latency = query['latency'] || 0; +/* + * Infinite scroll related endpoints. + */ +const randInt = n => { + return Math.floor(Math.random() * Math.floor(n)); +}; + +const squareImgUrl = width => { + return `http://picsum.photos/${width}?` + randInt(50); +}; +const generateJson = numberOfItems => { + const results = []; for (let i = 0; i < numberOfItems; i++) { - const imageUrl = 'http://picsum.photos/200?' + - Math.floor(Math.random() * Math.floor(50)); + const imageUrl = squareImgUrl(200); const r = { - 'title': 'Item ' + i, + 'title': 'Item ' + randInt(100), imageUrl, - 'price': i + 0.99, + 'price': randInt(8) + 0.99, }; - items.push(r); + results.push(r); } + return results; +}; + +app.get('/infinite-scroll-faulty', function(req, res) { + const {query} = req; + const code = query['code']; + const items = generateJson(12); + let next = '/infinite-scroll-error'; + if (code) { + next += '?code=' + code; + } + res.json({items, next}); +}); + +app.get('/infinite-scroll-error', function(req, res) { + const {query} = req; + const code = query['code'] || 404; + res.status(code); + res.json({'msg': code}); +}); + +app.get('/infinite-scroll', function(req, res) { + const {query} = req; + const numberOfItems = query['items'] || 10; + const pagesLeft = query['left'] || 1; + const latency = query['latency'] || 0; + + const items = generateJson(numberOfItems); const nextUrl = '/infinite-scroll?items=' + numberOfItems + '&left=' + (pagesLeft - 1) + @@ -1206,6 +1239,10 @@ app.get('/infinite-scroll', function(req, res) { 'loadMoreEndText': 'end', }; + if (pagesLeft < 3) { + res.status(500).json(results); + } + if (latency) { setTimeout(() => res.json(results), latency); } else { diff --git a/test/manual/amp-list/infinite-scroll-fail.amp.html b/test/manual/amp-list/infinite-scroll-fail.amp.html new file mode 100644 index 000000000000..c5dc8d3eab99 --- /dev/null +++ b/test/manual/amp-list/infinite-scroll-fail.amp.html @@ -0,0 +1,199 @@ + + + + + AMP #0 + + + + + + + + + + + + + + + + + + +
+

AMP List

+

This automatic load more example returns 16 items with a faulty next url. Change the error code below.

+
+ Error Code: + + +
+ + + +
+
+ See More +
+
+
+ +
+
+
+
+ +
+ Unable to Load More +
+
+ Retry +
+
+ + +
+ + \ No newline at end of file From 7ebe2c9e0a1cb3008526ca38faa5e7eb4af656ac Mon Sep 17 00:00:00 2001 From: Cathy Zhu Date: Mon, 10 Dec 2018 15:44:16 -0800 Subject: [PATCH 3/3] Address PR comments --- build-system/app.js | 6 +----- extensions/amp-list/0.1/amp-list.js | 3 +-- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/build-system/app.js b/build-system/app.js index e50619a0fe6d..9f08c312b9ce 100644 --- a/build-system/app.js +++ b/build-system/app.js @@ -1175,7 +1175,7 @@ const randInt = n => { }; const squareImgUrl = width => { - return `http://picsum.photos/${width}?` + randInt(50); + return `http://picsum.photos/${width}?${randInt(50)}`; }; const generateJson = numberOfItems => { @@ -1239,10 +1239,6 @@ app.get('/infinite-scroll', function(req, res) { 'loadMoreEndText': 'end', }; - if (pagesLeft < 3) { - res.status(500).json(results); - } - if (latency) { setTimeout(() => res.json(results), latency); } else { diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index cadc94172cc8..e9afece927f9 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -383,9 +383,8 @@ export class AmpList extends AMP.BaseElement { return fetch.catch(error => { if (opt_append) { throw error; - } else { - this.showFallback_(error); } + this.showFallback_(error); }); }