From 1dc62c71c69f46915217b14408dc1ff38575a5a8 Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Fri, 10 Aug 2018 14:23:44 -0700 Subject: [PATCH] Remove fetchDocument from xhr-impl.js (#17240) * 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 --- build-system/tasks/bundle-size.js | 2 +- src/service/xhr-impl.js | 18 ---- src/utils/xhr-utils.js | 19 +---- test/functional/test-batched-xhr.js | 45 ---------- test/functional/test-xhr.js | 124 ---------------------------- 5 files changed, 4 insertions(+), 204 deletions(-) diff --git a/build-system/tasks/bundle-size.js b/build-system/tasks/bundle-size.js index 4f770053cc1a..693f24c7b180 100644 --- a/build-system/tasks/bundle-size.js +++ b/build-system/tasks/bundle-size.js @@ -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; diff --git a/src/service/xhr-impl.js b/src/service/xhr-impl.js index 65821d392c7f..18d5a0669bb6 100644 --- a/src/service/xhr-impl.js +++ b/src/service/xhr-impl.js @@ -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} - */ - 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. diff --git a/src/utils/xhr-utils.js b/src/utils/xhr-utils.js index ec3352acbba6..3a74f02d44a5 100644 --- a/src/utils/xhr-utils.js +++ b/src/utils/xhr-utils.js @@ -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'); } @@ -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} * @private Visible for testing */ @@ -570,20 +571,6 @@ export class FetchResponse { this.drainText_().then(parseJson)); } - /** - * Reads the xhr responseXML. - * @return {!Promise} - */ - 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. diff --git a/test/functional/test-batched-xhr.js b/test/functional/test-batched-xhr.js index 689b5d5b10e5..7b276bc7e7b9 100644 --- a/test/functional/test-batched-xhr.js +++ b/test/functional/test-batched-xhr.js @@ -133,51 +133,6 @@ describes.sandboxed('BatchedXhr', {}, env => { }); }); - describes.realWin('#fetchDocument', {}, env => { - const TEST_CONTENT = 'Hello, world'; - const TEST_RESPONSE_TEXT = '' + 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 = { diff --git a/test/functional/test-xhr.js b/test/functional/test-xhr.js index be59be27ea9b..c1234a44074d 100644 --- a/test/functional/test-xhr.js +++ b/test/functional/test-xhr.js @@ -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', - }, - ''); - 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', - }, - '')); - 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', - }, - '')); - 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', - }, - '')); - 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; @@ -1070,22 +976,6 @@ describe.configure().skipSafari().run('XHR', function() { }); }); }); - - it('should return correct document response', () => { - sendMessageStub.returns( - Promise.resolve({ - body: 'Foo', - 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', () => { @@ -1120,20 +1010,6 @@ describe.configure().skipSafari().run('XHR', function() { }); }); - it('should return correct document response', () => { - sendMessageStub.returns( - Promise.resolve({ - body: 'Foo', - 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);