From 8651103e154bfe27bb87eb81bf28623d2d4b674e Mon Sep 17 00:00:00 2001 From: istateside Date: Mon, 8 Jan 2018 11:04:59 -0500 Subject: [PATCH 01/10] replaces `getPromise`and `get` with fetch API --- src/cropduster.js | 85 +++++++++++++++++++++-------------------------- 1 file changed, 37 insertions(+), 48 deletions(-) diff --git a/src/cropduster.js b/src/cropduster.js index cc27e2f..0882a20 100644 --- a/src/cropduster.js +++ b/src/cropduster.js @@ -191,65 +191,54 @@ const CD = { options = {}; } - return this.getPromise(url, options).then( - response => { - callback(response.data, response.status, response.contentType); - return response; - }, - ({ status }) => { - callback(null, status); - return null; + const msg = `XHR: ${url}`; + + const deprecatedCallback = function() { + if (callback && typeof callback === 'function') { + CD.log( + 'callbacks are deprecated in cropduster, prefer using promises with asynchronous operations' + ); + return callback(...arguments); } - ); - }, + }; - getPromise(url, options = {}) { - const msg = `xhr: ${url}`; + const handleError = error => { + CD.log(`Error encountered in CD.get for ${url}: ${error}`); + CD.resume(msg); - return new Promise(function(resolve, reject) { - try { - const req = new XMLHttpRequest(); + deprecatedCallback(null); - req.onerror = function() { - CD.resume(msg); - CD.log(`XHR error for ${url}`); + if (!error instanceof Error) { + error = new Error(error); + } - reject({ - status: this.status, - statusText: req.statusText - }); - }; + throw error; + }; - req.onload = function() { - CD.resume(msg); - const contentType = this.getResponseHeader('content-type'); + CD.pause(options.maxSuspension || 0, msg); - resolve({ - contentType, - data: this.responseText, - status: this.status - }); - }; + const requestOptions = CD._optionsForFetch(options); - req.open(options.method || 'GET', url, true); + return fetch(url, requestOptions).then( + response => { + if (!response.ok) { + handleError(response.statusText); // A non-200 range status was returned + } - req.withCredentials = true; + return response.text().then(data => { + deprecatedCallback(data, response.status, response.contentType); - if (options.headers) { - for (const header in options.headers) { - req.setRequestHeader(header, options.headers[header]); - } - } + CD.resume(msg); - req.send(options.body); - CD.pause(options.maxSuspension, msg); - } catch (error) { - reject({ - message: `Cropduster failed to create Promise: ${error}`, - error: error - }); - } - }); + return { + data, + status, + contentType + }; + }, handleError); // The response failed to parse with `.text()` + }, + handleError // A network or CORS error was hit + ); }, getImage(url, options = {}, callback = () => {}) { From b301eebeefa55f3a3ddd239e8da7bc149c536e76 Mon Sep 17 00:00:00 2001 From: istateside Date: Mon, 8 Jan 2018 12:46:45 -0500 Subject: [PATCH 02/10] installs fetch-mock and updates tests --- package.json | 1 + src/cropduster.js | 16 +++++- tests/tests.js | 136 +++++++++++++++++----------------------------- yarn.lock | 11 ++++ 4 files changed, 76 insertions(+), 88 deletions(-) diff --git a/package.json b/package.json index 1439a2f..62bf978 100644 --- a/package.json +++ b/package.json @@ -11,6 +11,7 @@ "babel-loader": "^7.1.2", "babel-plugin-add-module-exports": "^0.2.1", "babel-preset-env": "^1.6.1", + "fetch-mock": "^6.0.0-beta.7", "jquery": "^3.2.1", "karma": "^1.7.1", "karma-babel-preprocessor": "^7.0.0", diff --git a/src/cropduster.js b/src/cropduster.js index eff2d13..e3a6486 100644 --- a/src/cropduster.js +++ b/src/cropduster.js @@ -226,7 +226,10 @@ const CD = { } return response.text().then(data => { - deprecatedCallback(data, response.status, response.contentType); + const status = response.status; + const contentType = response.headers.get('Content-Type'); + + deprecatedCallback(data, status, contentType); CD.resume(msg); @@ -351,6 +354,17 @@ const CD = { } return hash.toString(); + }, + + _optionsForFetch(options) { + return { + credentials: 'include', + redirect: 'follow', + headers: new Headers(options.headers || {}), + method: options.method || 'GET', + body: options.body, + mode: 'cors' + }; } }; diff --git a/tests/tests.js b/tests/tests.js index f663163..c3d8e44 100644 --- a/tests/tests.js +++ b/tests/tests.js @@ -1,6 +1,7 @@ import CD from '../src/cropduster'; import jQuery from 'jquery'; -import { fakeServer, useFakeXMLHttpRequest, spy } from 'sinon'; +import { spy } from 'sinon'; +import fetchMock from 'fetch-mock'; const { module, test } = QUnit; @@ -8,6 +9,13 @@ const container = jQuery("
module('cropduster tests', { beforeEach() { + this.fakeServer = fetchMock.mock('*', { + body: 'ok', + headers: { + 'Content-Type': 'text/html' + } + }); + window.MICapture = { pause: spy(), resume: spy(), @@ -15,6 +23,10 @@ module('cropduster tests', { error: spy(), waitForAsset: spy() }; + }, + + afterEach() { + this.fakeServer.restore(); } }); @@ -222,56 +234,42 @@ test("CD.getImages with promises and a single callback", function(assert) { }); test("DEPRECATED - CD.get with callbacks and a successful response", function(assert) { - const xhr = useFakeXMLHttpRequest(); const done = assert.async(); - const server = fakeServer.create(); - server.respondWith([200, {"Content-Type": "text/html"}, "response"]); - - CD.get("http://google.com", { + CD.get("http://callback.com", { headers: { 'Accept': 'application/json' } }, (data, status, contentType) => { - assert.ok(data === 'response', 'resolves with a response'); + assert.ok(data === 'ok', 'resolves with a response'); assert.ok(status === 200, 'resolves with a status'); assert.ok(contentType === 'text/html', 'resolves with a content type'); assert.ok(true, 'the promise resolves successfully'); done(); }); - - server.respond(); - server.restore(); }); test("CD.get with promises and a successful response", function(assert) { - const xhr = useFakeXMLHttpRequest(); const done = assert.async(); - const server = fakeServer.create(); - - server.respondWith([200, {"Content-Type": "text/html"}, "response"]); - CD.get("http://google.com", { + CD.get("http://callback.com", { headers: { 'Accept': 'application/json' } }).then(({ data, status, contentType }) => { - assert.ok(data === 'response', 'resolves with a response'); + assert.ok(data === 'ok', 'resolves with a response'); assert.ok(status === 200, 'resolves with a status'); assert.ok(contentType === 'text/html', 'resolves with a content type'); assert.ok(true, 'the promise resolves successfully'); done(); }); - - server.respond(); - server.restore(); }); test("DEPRECATED - CD.get with callbacks and a failing response", function(assert) { - const xhr = useFakeXMLHttpRequest(); - const server = fakeServer.create(); + this.fakeServer.restore(); + this.fakeServer.mock('*', 500); const done = assert.async(); CD.get("http://google.com", { @@ -282,16 +280,11 @@ test("DEPRECATED - CD.get with callbacks and a failing response", function(asser assert.equal(value, null, 'the callback is called with null if the request fails'); done(); }); - - server.requests[0].abort(); - server.requests[0].onerror(); - - server.restore(); }); test("CD.get with promises and a failing response", function(assert) { - const xhr = useFakeXMLHttpRequest(); - const server = fakeServer.create(); + this.fakeServer.restore(); + this.fakeServer.mock('*', 500); const done = assert.async(); CD.get("http://google.com", { @@ -308,21 +301,10 @@ test("CD.get with promises and a failing response", function(assert) { done(); } ); - - server.requests[0].abort(); - server.requests[0].onerror(); - - server.restore(); }); test("CD.get - request options", function(assert) { - const xhr = useFakeXMLHttpRequest(); - const requests = this.requests = []; - xhr.onCreate = function (xhr) { - requests.push(xhr); - }; - CD.get("http://google.com", { corsCacheTime: 5000, headers: { @@ -330,24 +312,17 @@ test("CD.get - request options", function(assert) { } }); - assert.equal(requests.length, 1); - assert.equal(requests[0].requestHeaders['x-reverse-proxy-ttl'], null); // not automatically added - assert.equal(requests[0].requestHeaders['Accept'], 'application/json'); - assert.equal(requests[0].method, 'GET'); - assert.equal(requests[0].async, true); - assert.equal(requests[0].withCredentials, true); - assert.equal(requests[0].url, "http://google.com"); + const lastRequest = this.fakeServer.lastOptions(); - xhr.restore(); + assert.equal(this.fakeServer.calls.length, 1); + assert.equal(this.fakeServer.lastUrl(), "http://google.com"); + assert.equal(lastRequest.headers.get('x-reverse-proxy-ttl'), null); // not automatically added + assert.equal(lastRequest.headers.get('Accept'), 'application/json'); + assert.equal(lastRequest.method, 'GET'); + assert.equal(lastRequest.credentials, 'include'); }); test("CD.getCORS - request options", function(assert) { - const xhr = useFakeXMLHttpRequest(); - const requests = this.requests = []; - xhr.onCreate = function (xhr) { - requests.push(xhr); - }; - CD.getCORS("http://google.com", { corsCacheTime: 5000, headers: { @@ -355,41 +330,29 @@ test("CD.getCORS - request options", function(assert) { } }); - assert.equal(requests.length, 1); - assert.equal(requests[0].requestHeaders['x-reverse-proxy-ttl'], 5); - assert.equal(requests[0].requestHeaders['x-mi-cbe'], '-2134781906'); - assert.equal(requests[0].requestHeaders['Accept'], 'application/json'); - assert.equal(requests[0].method, 'GET'); - assert.equal(requests[0].async, true); - assert.equal(requests[0].url, "http://cors.movableink.com/google.com/"); + const lastRequest = this.fakeServer.lastOptions(); - xhr.restore(); + assert.equal(this.fakeServer.calls.length, 1); + assert.equal(this.fakeServer.lastUrl(), "http://cors.movableink.com/google.com/"); + assert.equal(lastRequest.headers.get('x-reverse-proxy-ttl'), 5); // not automatically added + assert.equal(lastRequest.headers.get('x-mi-cbe'), '-2134781906'); + assert.equal(lastRequest.headers.get('Accept'), 'application/json'); + assert.equal(lastRequest.method, 'GET'); + assert.equal(lastRequest.credentials, 'include'); }); test("CD.getCORS without options", function(assert) { - const xhr = useFakeXMLHttpRequest(); - const requests = this.requests = []; - xhr.onCreate = function (xhr) { - requests.push(xhr); - }; - CD.getCORS("http://google.com"); - assert.equal(requests.length, 1); - assert.equal(requests[0].requestHeaders['x-reverse-proxy-ttl'], 10); - assert.equal(requests[0].requestHeaders['x-mi-cbe'], '2084411041'); - assert.equal(requests[0].url, "http://cors.movableink.com/google.com/"); + const lastRequest = this.fakeServer.lastOptions(); - xhr.restore(); + assert.equal(this.fakeServer.calls.length, 1); + assert.equal(this.fakeServer.lastUrl(), "http://cors.movableink.com/google.com/"); + assert.equal(lastRequest.headers.get('x-reverse-proxy-ttl'), 10); + assert.equal(lastRequest.headers.get('x-mi-cbe'), '2084411041'); }); test("CD.getCORS with POST", function(assert) { - const xhr = useFakeXMLHttpRequest(); - const requests = this.requests = []; - xhr.onCreate = function (xhr) { - requests.push(xhr); - }; - CD.getCORS("http://google.com", { corsCacheTime: 5000, method: 'POST', @@ -399,16 +362,15 @@ test("CD.getCORS with POST", function(assert) { } }); - assert.equal(requests.length, 1); - assert.equal(requests[0].requestHeaders['x-reverse-proxy-ttl'], 5); - assert.equal(requests[0].requestHeaders['Accept'], 'application/json'); - assert.equal(requests[0].requestHeaders['x-mi-cbe'], '-1217831332'); - assert.equal(requests[0].method, 'POST'); - assert.equal(requests[0].async, true); - assert.equal(requests[0].requestBody, 'foobar'); - assert.equal(requests[0].url, "http://cors.movableink.com/google.com/"); + const lastRequest = this.fakeServer.lastOptions(); - xhr.restore(); + assert.equal(this.fakeServer.calls.length, 1); + assert.equal(this.fakeServer.lastUrl(), "http://cors.movableink.com/google.com/"); + assert.equal(lastRequest.headers.get('x-reverse-proxy-ttl'), 5); // not automatically added + assert.equal(lastRequest.headers.get('x-mi-cbe'), '-1217831332'); + assert.equal(lastRequest.headers.get('Accept'), 'application/json'); + assert.equal(lastRequest.method, 'POST'); + assert.equal(lastRequest.body, 'foobar'); }); test("_hashForRequest", function(assert) { diff --git a/yarn.lock b/yarn.lock index efc5126..2ecc2bc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1788,6 +1788,13 @@ faye-websocket@~0.11.0: dependencies: websocket-driver ">=0.5.1" +fetch-mock@^6.0.0-beta.7: + version "6.0.0-beta.7" + resolved "https://registry.yarnpkg.com/fetch-mock/-/fetch-mock-6.0.0-beta.7.tgz#d30956fbc8c8844fb4cbbf342969f36931562b7e" + dependencies: + glob-to-regexp "^0.3.0" + path-to-regexp "^1.7.0" + filename-regex@^2.0.0: version "2.0.1" resolved "https://registry.yarnpkg.com/filename-regex/-/filename-regex-2.0.1.tgz#c1c4b9bee3e09725ddb106b75c1e301fe2f18b26" @@ -2005,6 +2012,10 @@ glob-parent@^2.0.0: dependencies: is-glob "^2.0.0" +glob-to-regexp@^0.3.0: + version "0.3.0" + resolved "https://registry.yarnpkg.com/glob-to-regexp/-/glob-to-regexp-0.3.0.tgz#8c5a1494d2066c570cc3bfe4496175acc4d502ab" + glob@^7.0.3, glob@^7.0.5, glob@^7.1.1, glob@^7.1.2: version "7.1.2" resolved "https://registry.yarnpkg.com/glob/-/glob-7.1.2.tgz#c19c9df9a028702d678612384a6552404c636d15" From 8b52e7eb6e4eca83299f9e6efd2a0eed45e1d5ec Mon Sep 17 00:00:00 2001 From: istateside Date: Mon, 8 Jan 2018 12:57:12 -0500 Subject: [PATCH 03/10] adds README note --- README.md | 3 +++ package.json | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 3c1a0b2..6293902 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,9 @@ console.log('If user clicks on the web crop, they will go to http://example.com' ## Changelog +### 5.0.1 + * CD.get uses the `fetch` API instead of XMLHttpRequest + ### 5.0.0 * Compiles the app with Webpack and Babel down to ES5 with sourcemaps. * Returns Promises from CD.get, CD.getCORS, CD.getImage and CD.getImages diff --git a/package.json b/package.json index 62bf978..2c233b8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "cropduster", - "version": "5.0.0", + "version": "5.0.1", "description": "Library for building web pages for use with Movable Ink Web Crops", "main": "dist/cropduster.js", "directories": { From b8770ccbe5201d07af176607eb72bd4e47c6f9e5 Mon Sep 17 00:00:00 2001 From: istateside Date: Wed, 17 Jan 2018 13:18:56 -0500 Subject: [PATCH 04/10] bumps to 5.1.0 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 4831439..1f24ef6 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "cropduster", - "version": "5.0.1", + "version": "5.1.0", "description": "Library for building web pages for use with Movable Ink Web Crops", "main": "src/cropduster.js", "directories": { From ce1f094b29dadf63f8f6d26ea3ed2056c49b1c5f Mon Sep 17 00:00:00 2001 From: istateside Date: Wed, 17 Jan 2018 13:22:33 -0500 Subject: [PATCH 05/10] changes version in README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e68b72b..4bc2eea 100644 --- a/README.md +++ b/README.md @@ -218,7 +218,7 @@ console.log('If user clicks on the web crop, they will go to http://example.com' ## Changelog -### 5.0.1 +### 5.1.0 * CD.get uses the `fetch` API instead of XMLHttpRequest ### 5.0.0 From 06a66b63f5e5e8f0d2a3a39d1a4bad591c14e009 Mon Sep 17 00:00:00 2001 From: istateside Date: Thu, 18 Jan 2018 14:11:34 -0500 Subject: [PATCH 06/10] updates testing commands in readme --- README.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 4bc2eea..cdd35db 100644 --- a/README.md +++ b/README.md @@ -212,9 +212,8 @@ console.log('If user clicks on the web crop, they will go to http://example.com' ## Testing - brew install phantomjs - npm install - npm test + yarn install + yarn run test ## Changelog From 737c2f84e62640ad7ffb2051630140aea142d470 Mon Sep 17 00:00:00 2001 From: istateside Date: Thu, 18 Jan 2018 14:15:12 -0500 Subject: [PATCH 07/10] adds readme explanation for using CD.pause and resume next to each other --- README.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index cdd35db..66c6de4 100644 --- a/README.md +++ b/README.md @@ -64,12 +64,20 @@ if (customerQuality === 'very-good') { CD.pause(tenSeconds, 'making bad customers wait for their email to load...'); setTimeout(() => { - target.innerText = 'bad customers have to wait for their images'; CD.resume(); + target.innerText = 'bad customers have to wait for their images'; }, 1000); } ``` +It is generally recommended to call CD.pause as the last thing before starting +an asynchronous action, and CD.resume as the very first thing once that action +has finished. If your asynchronous action completes successfully, but your +callback runs some code before calling CD.resume again, then you run the risk of +triggering a JavaScript error that stops execution of the current script. If +CD.resume is not called after that first CD.pause, your image will eventually +time out, as opposed to failing immediately. + *NOTE:* Cropduster previously offered `CD.suspend` and `CD.capture` functions that achieved a similar goal. These functions have been replaced with `pause` and `resume`, to support a better synchronisation of state with Capturama, and From a4e8cfa9c46a693db6247ae2f81544c8fbbb133e Mon Sep 17 00:00:00 2001 From: istateside Date: Thu, 18 Jan 2018 16:01:14 -0500 Subject: [PATCH 08/10] simplifies CD.get function --- src/cropduster.js | 57 +++++++++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/src/cropduster.js b/src/cropduster.js index cdc6eae..d6dd328 100644 --- a/src/cropduster.js +++ b/src/cropduster.js @@ -197,45 +197,40 @@ const CD = { } }; - const handleError = error => { - CD.log(`Error encountered in CD.get for ${url}: ${error}`); - CD.resume(msg); - - deprecatedCallback(null); - - if (!error instanceof Error) { - error = new Error(error); - } - - throw error; - }; - CD.pause(options.maxSuspension || 0, msg); const requestOptions = CD._optionsForFetch(options); - return fetch(url, requestOptions).then( - response => { - if (!response.ok) { - handleError(response.statusText); // A non-200 range status was returned - } - - return response.text().then(data => { - const status = response.status; - const contentType = response.headers.get('Content-Type'); + return fetch(url, requestOptions).then(response => { + if (!response.ok) { + throw new Error(response.statusText); // A non-200 range status was returned + } - deprecatedCallback(data, status, contentType); + return response.text().then(data => { + const status = response.status; + const contentType = response.headers.get('Content-Type'); - CD.resume(msg); + deprecatedCallback(data, status, contentType); - return { - data, - status, - contentType - }; - }, handleError); // The response failed to parse with `.text()` + return { + data, + status, + contentType + }; + }); + }).then( + (response) => { + CD.resume(msg); + return response; }, - handleError // A network or CORS error was hit + (error) => { + CD.log(`Error encountered in CD.get for ${url}: ${error}`); + CD.resume(msg); + + deprecatedCallback(null); + + throw error; + } ); }, From 47114aaa582e4a69cf588d4a6a2d5cf75e228756 Mon Sep 17 00:00:00 2001 From: istateside Date: Mon, 22 Jan 2018 11:09:58 -0500 Subject: [PATCH 09/10] updates withoutCredentials attr to work with fetch API --- src/cropduster.js | 9 +++++++-- tests/tests.js | 15 ++++++++------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/cropduster.js b/src/cropduster.js index d6dd328..30f70bd 100644 --- a/src/cropduster.js +++ b/src/cropduster.js @@ -351,14 +351,19 @@ const CD = { }, _optionsForFetch(options) { - return { - credentials: 'include', + const requestOptions = { redirect: 'follow', headers: new Headers(options.headers || {}), method: options.method || 'GET', body: options.body, mode: 'cors' }; + + if (!options.withoutCredentials) { + requestOptions.credentials = 'include'; + } + + return requestOptions; } }; diff --git a/tests/tests.js b/tests/tests.js index 79f2fba..346fb21 100644 --- a/tests/tests.js +++ b/tests/tests.js @@ -345,13 +345,14 @@ test("CD.get - withoutCredentials option", function(assert) { withoutCredentials: true }); - assert.equal(requests.length, 1); - assert.equal(requests[0].requestHeaders['x-reverse-proxy-ttl'], null); // not automatically added - assert.equal(requests[0].requestHeaders['Accept'], 'application/json'); - assert.equal(requests[0].method, 'GET'); - assert.equal(requests[0].async, true); - assert.equal(requests[0].withCredentials, false); - assert.equal(requests[0].url, "http://google.com"); + const lastRequest = this.fakeServer.lastOptions(); + + assert.equal(this.fakeServer.calls.length, 1); + assert.equal(this.fakeServer.lastUrl(), "http://google.com"); + assert.equal(lastRequest.headers.get('x-reverse-proxy-ttl'), null); // not automatically added + assert.equal(lastRequest.headers.get('Accept'), 'application/json'); + assert.equal(lastRequest.method, 'GET'); + assert.equal(lastRequest.credentials, 'include'); xhr.restore(); }); From 11ea0fdb24f211ef06736d9dcf89280dc3e02938 Mon Sep 17 00:00:00 2001 From: istateside Date: Mon, 22 Jan 2018 11:12:58 -0500 Subject: [PATCH 10/10] fixes withoutCredentials test --- tests/tests.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/tests.js b/tests/tests.js index 346fb21..a2748c8 100644 --- a/tests/tests.js +++ b/tests/tests.js @@ -331,12 +331,6 @@ test("CD.get - request options", function(assert) { }); test("CD.get - withoutCredentials option", function(assert) { - const xhr = useFakeXMLHttpRequest(); - const requests = this.requests = []; - xhr.onCreate = function (xhr) { - requests.push(xhr); - }; - CD.get("http://google.com", { corsCacheTime: 5000, headers: { @@ -352,9 +346,8 @@ test("CD.get - withoutCredentials option", function(assert) { assert.equal(lastRequest.headers.get('x-reverse-proxy-ttl'), null); // not automatically added assert.equal(lastRequest.headers.get('Accept'), 'application/json'); assert.equal(lastRequest.method, 'GET'); - assert.equal(lastRequest.credentials, 'include'); - xhr.restore(); + assert.notOk('credentials' in lastRequest, 'the request does not have a "credentials" option set'); }); test("CD.getCORS - request options", function(assert) {