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

[dreamofabear]

Fixes #20638.

• Move fetch rejection handler up to the fetch promise, and use user().error instead of dev().error.
• Remove extraneous Promise between createHTMLDocument and attachment.

Tip: View diff with whitespace ignored (?w=1).

/to @peterjosling @sparhami

[sparhami]

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.

[dreamofabear]

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.

[sparhami]

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(...);
  }
});

[sparhami]

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

[dreamofabear]

Minimum change it is. :)

[sparhami]

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);
	}
	});
	}

[dreamofabear]

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.

[sparhami]

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(...);
  }
});

[sparhami]

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