Document-fetcher using utils functions #17206

prateekbh · 2018-07-31T00:08:52Z

Implements common functions (maybeIntercept_, fetchAmpCors_, fromStructuredCloneable_, toStructuredCloneable_) as utility functions.
xhr-impl and document-fetcher use these utility functions.
No one uses document-fetcher for now(this is just to keep the files to review count as low as possible).
Fixes Task Add commit hooks #1 in Move AMP runtime to standard fetch #16707

Next PR will shift all uses of fetchDocument to document-fetcher.

…hr-utils

prateekbh · 2018-07-31T16:30:17Z

@jridgewell i've split it xhr class into utility functions, and everything seems to work, please have a look

src/utils/xhr-utils.js

+ *     cloneable request.
+ * @private
+ */
+export function toStructuredCloneable(input, init) {


src/utils/xhr-utils.js

+ * @return {!FetchResponse|!Response} The deserialized regular response.
+ * @private
+ */
+export function fromStructuredCloneable(response, responseType) {


src/utils/xhr-utils.js

+  user().assert(isObject(response), 'Object expected: %s', response);
+
+  const isDocumentType = responseType == 'document';
+  if (typeof Response === 'function' && !isDocumentType) {


src/utils/xhr-utils.js

+ *     `Promise<undefined>` otherwise.
+ * @private
+ */
+export function getViewerInterceptResponse(win, ampdocSingle, input, init) {


src/utils/xhr-utils.js

+ * @param {!FetchInitDef} init The options of the XHR which may get
+ * intercepted.
+ */
+export function setupInput(win, input, init) {


prateekbh · 2018-07-31T19:33:21Z

src/utils/xhr-utils.js

+ * @param {string=} opt_accept The HTTP Accept header value.
+ * @return {!FetchInitDef}
+ */
+export function setupInit(opt_init, opt_accept) {


This piece needs review.
Its a mix of logic operating on init here: https://github.com/ampproject/amphtml/blob/master/src/service/xhr-impl.js#L139-L142

and in original setupInit function

prateekbh · 2018-07-31T19:34:26Z

src/utils/xhr-utils.js

+ * @param {?FetchInitDef=} init
+ * @return {!FetchInitDef}
+ */
+export function setupAMPCors(win, input, init) {


Needs review
a function forked from here: https://github.com/ampproject/amphtml/blob/master/src/service/xhr-impl.js#L375-L396

src/utils/xhr-utils.js

+ * @return {string}
+ * @private
+ */
+function normalizeMethod_(method) {


prateekbh · 2018-07-31T19:35:01Z

src/utils/xhr-utils.js

+ * @param {!../utils/xhr-utils.FetchInitDef=} init
+ * @return {!FetchResponse}
+ */
+export function verifyAmpCORSHeaders(win, response, init) {


a post processing function forked from here: https://github.com/ampproject/amphtml/blob/master/src/service/xhr-impl.js#L398-L414

src/utils/xhr-utils.js

+  return response;
+}
+
+// TODO(prateekbh): move everything below this line into the polyfill


jridgewell

Save my nits for a followup PR.

jridgewell · 2018-07-31T19:59:24Z

src/utils/xhr-utils.js

+ * @param {string=} opt_accept The HTTP Accept header value.
+ * @return {!FetchInitDef}
+ */
+export function setupInit(opt_init, opt_accept) {


jridgewell · 2018-07-31T20:01:17Z

src/utils/xhr-utils.js

+export function toStructuredCloneable(input, init) {
+  const newInit = Object.assign({}, init);
+  if (isFormDataWrapper(init.body)) {
+    newInit.headers = newInit.headers || {};


Nit: This should be guaranteed.

jridgewell · 2018-07-31T20:02:02Z

src/utils/xhr-utils.js

+      xhr.responseType = init.responseType;
+    }
+
+    if (init.headers) {


Nit: This should be guaranteed.

jridgewell · 2018-07-31T20:02:40Z

src/document-fetcher.js

@@ -0,0 +1,145 @@
+/**


Can we drop this file until the document fetch PR?

jridgewell · 2018-07-31T20:04:45Z

src/utils/xhr-utils.js

+  const currentOrigin = getWinOrigin(win);
+  const targetOrigin = parseUrlDeprecated(input).origin;
+  if (currentOrigin == targetOrigin) {
+    init['headers'] = init['headers'] || {};


Nit: This should be guaranteed.

jridgewell · 2018-07-31T20:05:08Z

src/utils/xhr-utils.js

+    init['headers']['AMP-Same-Origin'] = 'true';
+  }
+  // In edge a `TypeMismatchError` is thrown when body is set to null.
+  dev().assert(init.body !== null, 'fetch `body` can not be `null`');


Nit: seems like we could move this into setupInit.

jridgewell · 2018-07-31T20:05:16Z

src/utils/xhr-utils.js

+ * @param {?FetchInitDef=} init
+ * @return {!FetchInitDef}
+ */
+export function setupAMPCors(win, input, init) {


jridgewell · 2018-07-31T20:07:43Z

src/utils/xhr-utils.js

+ * @param {!../utils/xhr-utils.FetchInitDef=} init
+ * @return {!FetchResponse}
+ */
+export function verifyAmpCORSHeaders(win, response, init) {


…hr-utils

… xhr-utils

…hr-utils

codecov-io · 2018-08-02T05:20:08Z

Codecov Report

Merging #17206 into master will increase coverage by 0.01%.
The diff coverage is 93.5%.

@@            Coverage Diff             @@
##           master   #17206      +/-   ##
==========================================
+ Coverage   77.78%    77.8%   +0.01%     
==========================================
  Files         562      563       +1     
  Lines       41199    41205       +6     
==========================================
+ Hits        32048    32058      +10     
+ Misses       9151     9147       -4

Flag	Coverage Δ
#integration_tests	`36.1% <37.01%> (-0.02%)`	⬇️
#unit_tests	`76.85% <93.5%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85e69dc...d24b898. Read the comment docs.

prateekbh added 7 commits July 27, 2018 10:11

reverting skip

07ab01a

Merge branch 'master' of https://github.com/ampproject/amphtml

aeb82c7

Merge branch 'master' of https://github.com/ampproject/amphtml

e2c8a4c

Merge branch 'master' of https://github.com/ampproject/amphtml

b66c7dc

splitting util functions

443504a

bringing tests over

14c2c42

fixing types

515deb8

googlebot added the cla: yes label Jul 31, 2018

prateekbh added 2 commits July 30, 2018 17:29

fixing types

30f67ea

fixing types

97d3fea

prateekbh force-pushed the xhr-utils branch from bc75e3f to 97d3fea Compare July 31, 2018 00:49

fixing tests

337f5b2

prateekbh changed the title ~~[DO-NOT-REVIEW] document-fetcher using utils functions~~ Document-fetcher using utils functions Jul 31, 2018

Merge branch 'master' of https://github.com/ampproject/amphtml into x…

3e171af

…hr-utils

prateekbh force-pushed the xhr-utils branch from 229e25f to 2691e8d Compare July 31, 2018 16:01

making setupAMPCors as a function

57ead66

prateekbh force-pushed the xhr-utils branch from 2691e8d to 57ead66 Compare July 31, 2018 16:05

prateekbh requested a review from jridgewell July 31, 2018 16:30

prateekbh commented Jul 31, 2018

View reviewed changes

prateekbh force-pushed the xhr-utils branch from d5fe486 to 57ead66 Compare July 31, 2018 19:51

ampproject deleted a comment from prateekbh Jul 31, 2018

jridgewell approved these changes Jul 31, 2018

View reviewed changes

resolving comments

3f44499

fixing bundle-size

0b8e839

prateekbh mentioned this pull request Jul 31, 2018

DocumentFetcher class #16726

Closed

prateekbh added 3 commits July 31, 2018 15:17

Update bundle-size.js

3894d3b

Merge branch 'master' of https://github.com/ampproject/amphtml into x…

242e374

…hr-utils

Merge branch 'xhr-utils' of https://github.com/prateekbh/amphtml into…

b11268c

… xhr-utils

prateekbh requested a review from kristoferbaxter July 31, 2018 23:50

prateekbh added 3 commits August 1, 2018 09:37

fixing presubmit

f806270

Merge branch 'master' of https://github.com/ampproject/amphtml into x…

ac81386

…hr-utils

Merge branch 'master' of https://github.com/ampproject/amphtml into x…

d24b898

…hr-utils

prateekbh merged commit 940f1d3 into ampproject:master Aug 2, 2018

This was referenced Jul 29, 2021

🚨 Display: HTTP error 404 #35457

Closed

🚨 Display: HTTP error 404 #35458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document-fetcher using utils functions #17206

Document-fetcher using utils functions #17206

prateekbh commented Jul 31, 2018 •

edited

Loading

prateekbh commented Jul 31, 2018

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

prateekbh Jul 31, 2018

jridgewell Jul 31, 2018

prateekbh Jul 31, 2018

jridgewell Jul 31, 2018

This comment was marked as resolved.

prateekbh Jul 31, 2018

jridgewell Jul 31, 2018

This comment was marked as resolved.

jridgewell left a comment

jridgewell Jul 31, 2018

jridgewell Jul 31, 2018

jridgewell Jul 31, 2018

jridgewell Jul 31, 2018

prateekbh Jul 31, 2018

jridgewell Jul 31, 2018

jridgewell Jul 31, 2018

jridgewell Jul 31, 2018

jridgewell Jul 31, 2018

codecov-io commented Aug 2, 2018 •

edited

Loading

Document-fetcher using utils functions #17206

Document-fetcher using utils functions #17206

Conversation

prateekbh commented Jul 31, 2018 • edited Loading

prateekbh commented Jul 31, 2018

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

jridgewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 2, 2018 • edited Loading

Codecov Report

prateekbh commented Jul 31, 2018 •

edited

Loading

codecov-io commented Aug 2, 2018 •

edited

Loading