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.

Sign up for GitHub

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

Jump to bottom

amp-next-page: Fetch failure should be a user error #22293

Merged
merged 5 commits into from May 15, 2019
Merged

amp-next-page: Fetch failure should be a user error #22293

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
33e2c59
Fetch failure should be a user error.
May 14, 2019
0c48507
Minor clean up.
May 14, 2019
20fb1e9
Fix lint.
May 14, 2019
4cfdb81
Don't append empty docs.
May 15, 2019
fa1ae57
Just make the minimal change. :)
May 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 51 additions & 47 deletions extensions/amp-next-page/0.1/next-page-service.js
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@ import {
PositionObserverFidelity,
} from '../../../src/service/position-observer/position-observer-worker';
import {Services} from '../../../src/services';
import {dev, userAssert} from '../../../src/log';
import {dev, user, userAssert} from '../../../src/log';
import {dict} from '../../../src/utils/object';
import {getAmpdoc, getServiceForDoc} from '../../../src/service';
import {
Expand Down Expand Up @@ -237,7 +237,8 @@ export class NextPageService {
appendNextArticle_() {
if (this.nextArticle_ < this.config_.pages.length) {
const next = this.config_.pages[this.nextArticle_];
const documentRef = createDocumentRef(next.ampUrl);
const {ampUrl} = next;
const documentRef = createDocumentRef(ampUrl);
this.documentRefs_.push(documentRef);

const container = this.win_.document.createElement('div');
Expand Down Expand Up @@ -269,53 +270,56 @@ export class NextPageService {
}

this.nextArticle_++;
this.xhr_.fetch(next.ampUrl, {ampCors: false})
.then(response => {
// Update AMP URL in case we were redirected.
documentRef.ampUrl = response.url;
const url = this.urlService_.parse(response.url);
userAssert(url.origin === this.origin_,
'ampUrl resolved to a different origin from the origin of the '
+ 'current document');
return response.text();
})
.then(html => {
const doc =
this.win_.document.implementation.createHTMLDocument('');
doc.open();
doc.write(html);
doc.close();
return doc;
})
.then(doc => new Promise((resolve, reject) => {
if (documentRef.cancelled) {
// User has reached the end of the document already, don't render.
resolve();
return;
}
const fetchPromise = this.xhr_.fetch(ampUrl, {ampCors: false}).then(r => {
Copy link

Choose a reason for hiding this comment

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

Suggested change
const fetchPromise = this.xhr_.fetch(ampUrl, {ampCors: false}).then(r => {
this.fetchNextArticle_(next, documentRef)
.catch(e => user().error(TAG, 'Failed to fetch: %s', ampUrl, e))
.then(html => {
if (!html || documentRef.cancelled) {
// User has reached the end of the document already, don't render.
return;
}
if (documentRef.recUnit.isObserving) {
this.positionObserver_.unobserve(articleLinks);
documentRef.recUnit.isObserving = true;
}
return this.appendNextArticleHtml_(container, html);
})
// The new page may be short and the next may already need fetching.
.then(() => this.scrollHandler_());
}
fetchNextArticle(next, documentRef) {
return this.xhr_.fetch(next.ampUrl, {ampCors: false})
.then(response => {
// Update AMP URL in case we were redirected.
documentRef.ampUrl = response.url;
const url = this.urlService_.parse(response.url);
userAssert(url.origin === this.origin_,
'ampUrl resolved to a different origin from the origin of the '
+ 'current document');
return response.text();
});
}
appendNextArticleHtml(container, html) {
const doc = this.win_.document.implementation.createHTMLDocument('');
doc.open();
doc.write(html);
doc.close();
const shadowRoot = this.win_.document.createElement('div');
container.appendChild(shadowRoot);
return this.resources_.mutateElement(container, () => {
try {
const amp = this.attachShadowDoc_(shadowRoot, doc);
documentRef.amp = amp;
toggle(dev().assertElement(documentRef.recUnit.el), false);
this.documentQueued_ = false;
} catch (e) {
dev().error(TAG,
'failed to attach shadow document for %s', next.ampUrl, e);
}
});
}

Copy link
Author

Choose a reason for hiding this comment

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

appendNextArticleHtml needs to pass in documentRef and next.ampUrl too.

Personally I don't find this more readable e.g. both "fetch" and "append" mutate documentRef, so readers need to jump between methods to understand what's happening. That can be fixed with more reordering but it's more than I'd like for the scope of this PR.

// Update AMP URL in case we were redirected.
documentRef.ampUrl = r.url;
const url = this.urlService_.parse(r.url);
userAssert(url.origin === this.origin_,
'ampUrl resolved to a different origin from the origin of the '
+ 'current document');
return r.text();
}, e => {
user().error(TAG, 'Failed to fetch: %s', ampUrl, e);
});
// Once next page's HTML at `ampUrl` is fetched, inject it into a new doc.
fetchPromise.then(html => {
Copy link

Choose a reason for hiding this comment

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

I'm not sure this does what you want. The catch above will cause you to enter this .then with html being undefined, and isn't any different than doing

this.xhr_.fetch(...)
  .then(r => { ...}, e => {...})
  .then(html => [...});

Since this function is quite long already, I think this could be fixed and clarified by breaking out the attachment of the html part into a separate function. For example:

const fetchPromise = this.xhr_.fetch(...);

fetchPromise.catch(e => {...});
fetchPromise
  .then(r => {...})
  .then(html => {
     return appendArticleFromHtml(html);
  });

As-is, it is pretty hard to follow the flow of .then and .catch due to how long the code is.

The new code seems like it should attach a doc with the content 'undefined', which I don't think is the right thing to do for a failure.

The previous code looks to behave correctly by skipping all the attaching steps in the in the case of the fetch failure by letting the error fall through until the failed to fetch dev error.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, added an early return. 👍

Yea I couldn't resist the light refactoring because of readability. Looked into a "appendArticleWithHtml" helper but it'd need ~5 params so it doesn't feel like a readability win.

Copy link

Choose a reason for hiding this comment

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

Added a suggestion on how to refactor this above. You don't really need to pass too many arguments, if you separate out concerns here.

There are also a few minor things to clean up. For example, there is no need to wrap the mutate element with a try/catch, because the catch only is only ever logging an error and recovering into a non-failed state (assuming the unobserve doesn't throw an error), you could just do:

if (documentRef.cancelled) {
  return;
}

...

return this.resources_.mutateElement(container, () => {
  try {
    ...
  } catch (e) {
    dev().error(...);
  }
});

Copy link

Choose a reason for hiding this comment

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

I would prefer either this is code is refactored more broadly, or the minimum change needed is made in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Minimum change it is. :)

if (!html) {
return;
}
const doc = this.win_.document.implementation.createHTMLDocument('');
doc.open();
doc.write(html);
doc.close();

return new Promise((resolve, reject) => {
if (documentRef.cancelled) {
// User has reached the end of the document already, don't render.
resolve();
return;
}
if (documentRef.recUnit.isObserving) {
this.positionObserver_.unobserve(articleLinks);
documentRef.recUnit.isObserving = true;
}
this.resources_.mutateElement(container, () => {
try {
const amp = this.attachShadowDoc_(shadowRoot, doc);
documentRef.amp = amp;

if (documentRef.recUnit.isObserving) {
this.positionObserver_.unobserve(articleLinks);
documentRef.recUnit.isObserving = true;
toggle(dev().assertElement(documentRef.recUnit.el), false);
this.documentQueued_ = false;
resolve();
} catch (e) {
reject(e);
}
this.resources_.mutateElement(container, () => {
try {
const amp = this.attachShadowDoc_(shadowRoot, doc);
documentRef.amp = amp;

toggle(dev().assertElement(documentRef.recUnit.el), false);
this.documentQueued_ = false;
resolve();
} catch (e) {
reject(e);
}
});
}),
e => dev().error(TAG, 'failed to fetch %s', next.ampUrl, e))
.catch(e => dev().error(TAG,
'failed to attach shadow document for %s', next.ampUrl, e))
// The new page may be short and the next may already need fetching.
.then(() => this.scrollHandler_());
});
});
}).catch(e => {
dev().error(TAG, 'Failed to attach shadow document: %s', ampUrl, e);
}).then(() => {
// The new page may be short and the next may already need fetching.
this.scrollHandler_();
});
}
}

Expand Down