Add document fetcher #17259
Add document fetcher #17259
Conversation
…dd-document-fetcher
Codecov Report
@@ Coverage Diff @@
## master #17259 +/- ##
==========================================
- Coverage 77.71% 77.46% -0.25%
==========================================
Files 563 566 +3
Lines 41201 41491 +290
==========================================
+ Hits 32018 32143 +125
- Misses 9183 9348 +165
Continue to review full report at Codecov.
|
src/document-fetcher.js
Outdated
| * @param {!./utils/xhr-utils.FetchInitDef} init | ||
| * @private | ||
| */ | ||
| function xhrRequest_(input, init) { |
There was a problem hiding this comment.
Nit: local names shouldn't end in _, only private property names.
src/document-fetcher.js
Outdated
| init = setupAMPCors(win, input, init); | ||
| input = setupInput(win, input, init); | ||
| const ampdocService = Services.ampdocServiceFor(win); | ||
| const ampdocSingle_ = |
There was a problem hiding this comment.
Nit: local names shouldn't end in _, only private property names.
| export function fetchDocument(win, input, opt_init) { | ||
| let init = setupInit(opt_init, 'text/html'); | ||
| init = setupAMPCors(win, input, init); | ||
| input = setupInput(win, input, init); |
There was a problem hiding this comment.
Are these three always run like this? If so, probably means it should be joined into one function.
src/document-fetcher.js
Outdated
| xhr.responseType = 'document'; | ||
| // Incoming headers are in fetch format, | ||
| // so we need to convert them into xhr. | ||
| if (init.headers) { |
There was a problem hiding this comment.
init.headers is guaranteed.
src/document-fetcher.js
Outdated
| }; | ||
| const body = 'response' in xhr | ||
| ? xhr.response : xhr.responseText; | ||
| const response = new Response(/** @type {string} */ (body || ''), /** @type {!ResponseInit} */ (options)); |
There was a problem hiding this comment.
So this is all being done because verifyAmpCORSHeaders expects a response object with type signature { headers: { get() {} } }. We could slim all this down if it just pass in the header in question, or if it took just a headers object { get() {} }.
There was a problem hiding this comment.
Actually this is done because at the end of all the change(including fetch polyfill), everything should talk in terms of native Response, Request. So we'd have to construct Response to talk to assertSuccess, verifyAmpCORSHeaders etc.
There was a problem hiding this comment.
I disagree. Sure, we can make things that need a response to work (like, getting the response's body) use a Response. But verifyAmpCORSHeaders only needs a Headers (and only really needs { get() {} }. Simplifying the type requirements makes this code easier to reason about, because we're never going to use the response's body (xhr.reponseText) here, we aren't going to use status or statusText.
I really think we can simplify code by resolving with just the necessities here:
const makeshift = {
xml: xhr.responseXML,
// We can make this a wrapper class if we want to provide the full API
headers: {
get(header) {
return xhr.getResponseHeader(header);
},
},
};
resolve(makeshift);
// back to https://github.com/ampproject/amphtml/pull/17259/files#diff-71317e0e89806231df17433535139205R45
// callback parameter is already a struct, not a Response
return xhrRequest(input, init).then(({xml, headers}) => {
verifyAmpCORSHeaders(win, headers, init);
return xml;
});There was a problem hiding this comment.
Woops, I actually noticed something.
Till this PR nobody has polyfilled Response for IE11 and other browsers so I am changing this to FetchResponse anyways.
But I will come back to this after doing the polyfill PR.
Note for then:
verifyAmpCORSHeaders requires the headers part
assertSuccess requires the .ok. So either we make both of them not take response arguments at all or we construct the Response once and use it everywhere.
src/document-fetcher.js
Outdated
| response, | ||
| xhr, | ||
| }); | ||
| }).catch(e => reject(e)); |
There was a problem hiding this comment.
Nit: Just resolve with the promise, no need for another closure to handle reject:
const promise = assertSuccess(response).then(response => {
return { response, xhr };
});
resolve(promise);This is 99% equivalent (technically, yours would catch an error thrown by the resolve call, but that can't happen).
src/document-fetcher.js
Outdated
| * @return {JsonObject} | ||
| */ | ||
| function parseHeaders(rawHeaders) { | ||
| const headers = dict({}); |
There was a problem hiding this comment.
This was all done to satisfy Response's options { headers: !Object }, right?
Back to the type signature comment: If we change it to accept just a { get() {} } we could continue to use the old headers polyfill, which is much simpler/smaller. Or, if we pass the header directly into verifyAmpCORSHeaders, we don't need any of it.
There was a problem hiding this comment.
My real hesitation here is creating a parser for something I don't know much about. And this implementation isn't really efficient...
|
@jridgewell I moved from |
…d-document-fetcher
…dd-document-fetcher
* reverting skip * splitting util functions * bringing tests over * fixing types * fixing types * fixing types * fixing tests * making setupAMPCors as a function * resolving comments * fixing bundle-size * Update bundle-size.js * fixing presubmit * adding document fetcher * resolving comments * falling back to fetchResponse
* reverting skip * splitting util functions * bringing tests over * fixing types * fixing types * fixing types * fixing tests * making setupAMPCors as a function * resolving comments * fixing bundle-size * Update bundle-size.js * fixing presubmit * adding document fetcher * resolving comments * falling back to fetchResponse
document-fetcher.js