Skip to content

Commit

Permalink
Remove fetchDocument from xhr-impl.js (ampproject#17240)
Browse files Browse the repository at this point in the history 
* 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

* integrating fetchDocument with next-page service

* removing older fetchDocument functions

* WIP integgrating amp-access

* fixing amp-access tests

* bug fixes

* fixing live list manager tests

* adding todos

* removing unwanted tests

* removing tests for fetchDocument from batched XHR

* resolving comments

* Update test-amp-access-server-jwt.js

* Update test-amp-access-server.js

* Update test-live-list-manager.js

* Update test-live-list-manager.js

* Update test-amp-next-page.js

* Update test-xhr-document-fetcher.js

* Update test-xhr-document-fetcher.js

* reducing bundle-size

* Update bundle-size.js
  • Loading branch information
@prateekbh
prateekbh authored Aug 10, 2018
1 parent a2832f1 commit 1dc62c7
Show file tree
Hide file tree
Showing 5 changed files with 4 additions and 204 deletions.
2 changes: 1 addition & 1 deletion build-system/tasks/bundle-size.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const log = require('fancy-log');
const {getStdout} = require('../exec');

const runtimeFile = './dist/v0.js';
const maxSize = '80.63KB';
const maxSize = '80.54KB';

const {green, red, cyan, yellow} = colors;

Expand Down
18 changes: 0 additions & 18 deletions src/service/xhr-impl.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,24 +193,6 @@ export class Xhr {
return this.fetch(input, setupInit(opt_init, 'text/plain'));
}

/**
* Creates an XHR request with responseType=document
* and returns a promise for the initialized `Document`.
* Note this does not return a `Response`, since this is not a standard
* Fetch response type.
*
* @param {string} input
* @param {?../utils/xhr-utils.FetchInitDef=} opt_init
* @return {!Promise<!Document>}
*/
fetchDocument(input, opt_init) {
const init = setupInit(opt_init, 'text/html');
init.responseType = 'document';
return this.fetch(input, init)
.then(response =>
/** @type {!../utils/xhr-utils.FetchResponse} */(response).document());
}

/**
* @param {string} input URL
* @param {?../utils/xhr-utils.FetchInitDef=} opt_init Fetch options object.
Expand Down
19 changes: 3 additions & 16 deletions src/utils/xhr-utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,9 @@ export function fromStructuredCloneable(response, responseType) {
}
}

// TODO(prateekbh): remove responseXML after everything is moved to polyfill
// it's not used right now, but its tough to remove this due to typings.
if (isDocumentType) {
// TODO(prateekbh): remove this when integrating document fetcher.
data.responseXML =
new DOMParser().parseFromString(data.responseText, 'text/html');
}
Expand Down Expand Up @@ -482,7 +483,7 @@ function isRetriable(status) {

/**
* Returns the response if successful or otherwise throws an error.
* @param {!FetchResponse} response
* @param {!FetchResponse|!Response} response
* @return {!Promise<!FetchResponse>}
* @private Visible for testing
*/
Expand Down Expand Up @@ -570,20 +571,6 @@ export class FetchResponse {
this.drainText_().then(parseJson));
}

/**
* Reads the xhr responseXML.
* @return {!Promise<!Document>}
*/
document() {
dev().assert(!this.bodyUsed, 'Body already used');
this.bodyUsed = true;
user().assert(this.xhr_.responseXML,
'responseXML should exist. Make sure to return ' +
'Content-Type: text/html header.');
const doc = /** @type {!Document} */(dev().assert(this.xhr_.responseXML));
return Promise.resolve(doc);
}

/**
* Drains the response and returns a promise that resolves with the response
* ArrayBuffer.
Expand Down
45 changes: 0 additions & 45 deletions test/functional/test-batched-xhr.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,51 +133,6 @@ describes.sandboxed('BatchedXhr', {}, env => {
});
});

describes.realWin('#fetchDocument', {}, env => {
const TEST_CONTENT = '<b>Hello, world';
const TEST_RESPONSE_TEXT = '<!doctype html><html><body>' + TEST_CONTENT;
let xhr;
let fetchStub;
let testResponseDoc;

beforeEach(() => {
const doc = env.win.document;
testResponseDoc = doc.implementation.createHTMLDocument();
testResponseDoc.body.innerHTML = TEST_CONTENT;
const mockXhr = {
status: 200,
responseText: TEST_RESPONSE_TEXT,
responseXML: testResponseDoc,
responseType: 'text/html',
};
xhr = batchedXhrServiceForTesting(env.win);
fetchStub = env.sandbox.stub(xhr, 'fetchAmpCors_').callsFake(
() => Promise.resolve(new FetchResponse(mockXhr)));
});

it('should fetch document GET requests once for identical URLs', () => {
return Promise.all([
xhr.fetchDocument('/get?k=v1'),
xhr.fetchDocument('/get?k=v1'),
]).then(results => {
expect(fetchStub).to.be.calledOnce;
expect(results[0].isEqualNode(testResponseDoc)).to.be.true;
expect(results[1].isEqualNode(testResponseDoc)).to.be.true;
});
});

it('should not cache for POST requests', () => {
return Promise.all([
xhr.fetchDocument('/get?k=v1', {method: 'POST', body: {}}),
xhr.fetchDocument('/get?k=v1', {method: 'POST', body: {}}),
]).then(results => {
expect(fetchStub).to.be.calledTwice;
expect(results[0].isEqualNode(testResponseDoc)).to.be.true;
expect(results[1].isEqualNode(testResponseDoc)).to.be.true;
});
});
});

describes.fakeWin('#fetchText', {}, env => {
const TEST_RESPONSE = 'Hello, world!';
const mockXhr = {
Expand Down
124 changes: 0 additions & 124 deletions test/functional/test-xhr.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,100 +474,6 @@ describe.configure().skipSafari().run('XHR', function() {
});
});

describe('#fetchDocument', () => {
beforeEach(() => xhr = xhrServiceForTesting(test.win));

it('should be able to fetch a document', () => {
setupMockXhr();
const promise = xhr.fetchDocument('/index.html').then(doc => {
expect(doc.nodeType).to.equal(9);
});
xhrCreated.then(xhr => {
expect(xhr.requestHeaders['Accept']).to.equal('text/html');
xhr.respond(
200, {
'Content-Type': 'text/xml',
'Access-Control-Expose-Headers':
'AMP-Access-Control-Allow-Source-Origin',
'AMP-Access-Control-Allow-Source-Origin': 'https://acme.com',
},
'<html></html>');
expect(xhr.responseType).to.equal('document');
});
return promise;
});

it('should mark 400 as not retriable', () => {
setupMockXhr();
const promise = xhr.fetchDocument('/index.html');
xhrCreated.then(
xhr => xhr.respond(
400, {
'Content-Type': 'text/xml',
'AMP-Access-Control-Allow-Source-Origin': 'https://acme.com',
},
'<html></html>'));
return promise.catch(e => {
expect(e.retriable).to.be.equal(false);
expect(e.retriable).to.not.equal(true);
});
});

it('should mark 415 as retriable', () => {
setupMockXhr();
const promise = xhr.fetchDocument('/index.html');
xhrCreated.then(
xhr => xhr.respond(
415, {
'Content-Type': 'text/xml',
'Access-Control-Expose-Headers':
'AMP-Access-Control-Allow-Source-Origin',
'AMP-Access-Control-Allow-Source-Origin': 'https://acme.com',
},
'<html></html>'));
return promise.catch(e => {
expect(e.retriable).to.exist;
expect(e.retriable).to.be.true;
});
});

it('should mark 500 as retriable', () => {
setupMockXhr();
const promise = xhr.fetchDocument('/index.html');
xhrCreated.then(
xhr => xhr.respond(
415, {
'Content-Type': 'text/xml',
'Access-Control-Expose-Headers':
'AMP-Access-Control-Allow-Source-Origin',
'AMP-Access-Control-Allow-Source-Origin': 'https://acme.com',
},
'<html></html>'));
return promise.catch(e => {
expect(e.retriable).to.exist;
expect(e.retriable).to.be.true;
});
});

it('should error on non truthy responseXML', () => {
setupMockXhr();
const promise = xhr.fetchDocument('/index.html');
xhrCreated.then(
xhr => xhr.respond(
200, {
'Content-Type': 'application/json',
'Access-Control-Expose-Headers':
'AMP-Access-Control-Allow-Source-Origin',
'AMP-Access-Control-Allow-Source-Origin': 'https://acme.com',
},
'{"hello": "world"}'));
return promise.catch(e => {
expect(e.message)
.to.contain('responseXML should exist');
});
});
});

describe('#fetchText', () => {
const TEST_TEXT = 'test text';
let fetchStub;
Expand Down Expand Up @@ -1070,22 +976,6 @@ describe.configure().skipSafari().run('XHR', function() {
});
});
});

it('should return correct document response', () => {
sendMessageStub.returns(
Promise.resolve({
body: '<html><body>Foo</body></html>',
init: {
headers: [['AMP-Access-Control-Allow-Source-Origin', origin]],
},
}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetchDocument('https://www.some-url.org/some-resource/').then(doc => {
expect(doc).to.have.nested.property('body.textContent')
.that.equals('Foo');
});
});
});

describe('when native Response type is unavailable', () => {
Expand Down Expand Up @@ -1120,20 +1010,6 @@ describe.configure().skipSafari().run('XHR', function() {
});
});

it('should return correct document response', () => {
sendMessageStub.returns(
Promise.resolve({
body: '<html><body>Foo</body></html>',
init: {
headers: [['AMP-Access-Control-Allow-Source-Origin', origin]],
},
}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetchDocument('https://www.some-url.org/some-resource/')
.then(doc => expect(doc.body.textContent).to.equal('Foo'));
});

it('should return default response when body/init missing', () => {
sendMessageStub.returns(Promise.resolve({}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);
Expand Down

0 comments on commit 1dc62c7

Please sign in to comment.