reduce code duplication in makeRequest

mapbox · Oct 9, 2018 · 932241f · 932241f
1 parent 2000804
commit 932241f
Show file tree

Hide file tree

Showing 10 changed files with 78 additions and 112 deletions.
diff --git a/src/source/geojson_worker_source.js b/src/source/geojson_worker_source.js
@@ -20,7 +20,7 @@ import type Actor from '../util/actor';
 import type StyleLayerIndex from '../style/style_layer_index';
 
 import type {LoadVectorDataCallback} from './vector_tile_worker_source';
-import type {RequestParameters} from '../util/ajax';
+import type { RequestParameters, ResponseCallback } from '../util/ajax';
 import type { Callback } from '../types/callback';
 import type {GeoJSONFeature} from '@mapbox/geojson-types';
 
@@ -33,7 +33,7 @@ export type LoadGeoJSONParameters = {
     geojsonVtOptions?: Object
 };
 
-export type LoadGeoJSON = (params: LoadGeoJSONParameters, callback: Callback<mixed>) => void;
+export type LoadGeoJSON = (params: LoadGeoJSONParameters, callback: ResponseCallback<Object>) => void;
 
 export interface GeoJSONIndex {
     getTile(z: number, x: number, y: number): Object;
@@ -161,7 +161,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
         const perf = (params && params.request && params.request.collectResourceTiming) ?
             new performance.Performance(params.request) : false;
 
-        this.loadGeoJSON(params, (err, data) => {
+        this.loadGeoJSON(params, (err: ?Error, data: ?Object) => {
             if (err || !data) {
                 return callback(err);
             } else if (typeof data !== 'object') {
@@ -254,7 +254,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
      * @param [params.url] A URL to the remote GeoJSON data.
      * @param [params.data] Literal GeoJSON data. Must be provided if `params.url` is not.
      */
-    loadGeoJSON(params: LoadGeoJSONParameters, callback: Callback<mixed>) {
+    loadGeoJSON(params: LoadGeoJSONParameters, callback: ResponseCallback<Object>) {
         // Because of same origin issues, urls must either include an explicit
         // origin or absolute path.
         // ie: /foo/bar.json or http://example.com/bar.json

diff --git a/src/source/load_tilejson.js b/src/source/load_tilejson.js
@@ -12,7 +12,7 @@ import type {TileJSON} from '../types/tilejson';
 import type {Cancelable} from '../types/cancelable';
 
 export default function(options: any, requestTransformFn: RequestTransformFunction, callback: Callback<TileJSON>): Cancelable {
-    const loaded = function(err, tileJSON: any) {
+    const loaded = function(err: ?Error, tileJSON: ?Object) {
         if (err) {
             return callback(err);
         } else if (tileJSON) {

diff --git a/src/source/vector_tile_worker_source.js b/src/source/vector_tile_worker_source.js
@@ -1,6 +1,6 @@
 // @flow
 
-import {getArrayBuffer} from '../util/ajax';
+import { getArrayBuffer } from '../util/ajax';
 
 import vt from '@mapbox/vector-tile';
 import Protobuf from 'pbf';
@@ -43,15 +43,15 @@ export type LoadVectorData = (params: WorkerTileParameters, callback: LoadVector
  * @private
  */
 function loadVectorTile(params: WorkerTileParameters, callback: LoadVectorDataCallback) {
-    const request = getArrayBuffer(params.request, (err, response) => {
+    const request = getArrayBuffer(params.request, (err: ?Error, data: ?ArrayBuffer, cacheControl: ?string, expires: ?string) => {
         if (err) {
             callback(err);
-        } else if (response) {
+        } else if (data) {
             callback(null, {
-                vectorTile: new vt.VectorTile(new Protobuf(response.data)),
-                rawData: response.data,
-                cacheControl: response.cacheControl,
-                expires: response.expires
+                vectorTile: new vt.VectorTile(new Protobuf(data)),
+                rawData: data,
+                cacheControl: cacheControl,
+                expires: expires
             });
         }
     });

diff --git a/src/style/load_glyph_range.js b/src/style/load_glyph_range.js
@@ -3,6 +3,7 @@
 import { normalizeGlyphsURL } from '../util/mapbox';
 
 import { getArrayBuffer, ResourceType } from '../util/ajax';
+
 import parseGlyphPBF from './parse_glyph_pbf';
 
 import type {StyleGlyph} from './style_glyph';
@@ -23,13 +24,13 @@ export default function (fontstack: string,
             .replace('{range}', `${begin}-${end}`),
         ResourceType.Glyphs);
 
-    getArrayBuffer(request, (err, response) => {
+    getArrayBuffer(request, (err: ?Error, data: ?ArrayBuffer) => {
         if (err) {
             callback(err);
-        } else if (response) {
+        } else if (data) {
             const glyphs = {};
 
-            for (const glyph of parseGlyphPBF(response.data)) {
+            for (const glyph of parseGlyphPBF(data)) {
                 glyphs[glyph.id] = glyph;
             }
 

diff --git a/src/style/load_sprite.js b/src/style/load_sprite.js
@@ -17,7 +17,7 @@ export default function(baseURL: string,
     let json: any, image, error;
     const format = browser.devicePixelRatio > 1 ? '@2x' : '';
 
-    let jsonRequest = getJSON(transformRequestCallback(normalizeSpriteURL(baseURL, format, '.json'), ResourceType.SpriteJSON), (err, data) => {
+    let jsonRequest = getJSON(transformRequestCallback(normalizeSpriteURL(baseURL, format, '.json'), ResourceType.SpriteJSON), (err: ?Error, data: ?Object) => {
         jsonRequest = null;
         if (!error) {
             error = err;

diff --git a/src/style/style.js b/src/style/style.js
@@ -188,12 +188,12 @@ class Style extends Evented {
         url = normalizeStyleURL(url, options.accessToken);
         const request = this.map._transformRequest(url, ResourceType.Style);
 
-        this._request = getJSON(request, (error, json) => {
+        this._request = getJSON(request, (error: ?Error, json: ?Object) => {
             this._request = null;
             if (error) {
                 this.fire(new ErrorEvent(error));
             } else if (json) {
-                this._load((json: any), validate);
+                this._load(json, validate);
             }
         });
     }

diff --git a/src/util/ajax.js b/src/util/ajax.js
@@ -39,10 +39,14 @@ export type RequestParameters = {
     url: string,
     headers?: Object,
     method?: 'GET' | 'POST' | 'PUT',
+    body?: string,
+    type?: 'string' | 'json' | 'arraybuffer',
     credentials?: 'same-origin' | 'include',
     collectResourceTiming?: boolean
 };
 
+export type ResponseCallback<T> = (error: ?Error, data: ?T, cacheControl: ?string, expires: ?string) => void;
+
 class AJAXError extends Error {
     status: number;
     url: string;
@@ -61,32 +65,35 @@ class AJAXError extends Error {
     }
 }
 
-function makeRequest(requestParameters: RequestParameters): XMLHttpRequest {
+function makeRequest(requestParameters: RequestParameters, callback: ResponseCallback<any>): Cancelable {
     const xhr: XMLHttpRequest = new window.XMLHttpRequest();
 
     xhr.open(requestParameters.method || 'GET', requestParameters.url, true);
+    if (requestParameters.type === 'arraybuffer') {
+        xhr.responseType = 'arraybuffer';
+    }
     for (const k in requestParameters.headers) {
         xhr.setRequestHeader(k, requestParameters.headers[k]);
     }
+    if (requestParameters.type === 'json') {
+        xhr.setRequestHeader('Accept', 'application/json');
+    }
     xhr.withCredentials = requestParameters.credentials === 'include';
-    return xhr;
-}
-
-export const getJSON = function(requestParameters: RequestParameters, callback: Callback<mixed>): Cancelable {
-    const xhr = makeRequest(requestParameters);
-    xhr.setRequestHeader('Accept', 'application/json');
-    xhr.onerror = function() {
+    xhr.onerror = () => {
         callback(new Error(xhr.statusText));
     };
-    xhr.onload = function() {
-        if (((xhr.status >= 200 && xhr.status < 300) || xhr.status === 0) && xhr.response) {
-            let data;
-            try {
-                data = JSON.parse(xhr.response);
-            } catch (err) {
-                return callback(err);
+    xhr.onload = () => {
+        if (((xhr.status >= 200 && xhr.status < 300) || xhr.status === 0) && xhr.response !== null) {
+            let data: mixed = xhr.response;
+            if (requestParameters.type === 'json') {
+                // We're manually parsing JSON here to get better error messages.
+                try {
+                    data = JSON.parse(xhr.response);
+                } catch (err) {
+                    return callback(err);
+                }
             }
-            callback(null, data);
+            callback(null, data, xhr.getResponseHeader('Cache-Control'), xhr.getResponseHeader('Expires'));
         } else {
             if (xhr.status === 401 && requestParameters.url.match(/mapbox.com/)) {
                 callback(new AJAXError(`${xhr.statusText}: you may have provided an invalid Mapbox access token. See https://www.mapbox.com/api-documentation/#access-tokens`, xhr.status, requestParameters.url));
@@ -95,50 +102,20 @@ export const getJSON = function(requestParameters: RequestParameters, callback:
             }
         }
     };
-    xhr.send();
+    xhr.send(requestParameters.body);
     return { cancel: () => xhr.abort() };
-};
+}
 
-export const getArrayBuffer = function(requestParameters: RequestParameters, callback: Callback<{data: ArrayBuffer, cacheControl: ?string, expires: ?string}>): Cancelable {
-    const xhr = makeRequest(requestParameters);
-    xhr.responseType = 'arraybuffer';
-    xhr.onerror = function() {
-        callback(new Error(xhr.statusText));
-    };
-    xhr.onload = function() {
-        const response: ArrayBuffer = xhr.response;
-        if (response.byteLength === 0 && xhr.status === 200) {
-            return callback(new Error('http status 200 returned without content.'));
-        }
-        if (((xhr.status >= 200 && xhr.status < 300) || xhr.status === 0) && xhr.response) {
-            callback(null, {
-                data: response,
-                cacheControl: xhr.getResponseHeader('Cache-Control'),
-                expires: xhr.getResponseHeader('Expires')
-            });
-        } else {
-            callback(new AJAXError(xhr.statusText, xhr.status, requestParameters.url));
-        }
-    };
-    xhr.send();
-    return { cancel: () => xhr.abort() };
+export const getJSON = function(requestParameters: RequestParameters, callback: ResponseCallback<Object>): Cancelable {
+    return makeRequest(extend(requestParameters, { type: 'json' }), callback);
 };
 
-export const postData = function(requestParameters: RequestParameters, payload: string, callback: Callback<mixed>): Cancelable {
-    const xhr = makeRequest(extend(requestParameters, {method: 'POST'}));
+export const getArrayBuffer = function(requestParameters: RequestParameters, callback: ResponseCallback<ArrayBuffer>): Cancelable {
+    return makeRequest(extend(requestParameters, { type: 'arraybuffer' }), callback);
+};
 
-    xhr.onerror = function() {
-        callback(new Error(xhr.statusText));
-    };
-    xhr.onload = function() {
-        if (xhr.status >= 200 && xhr.status < 300) {
-            callback(null, xhr.response);
-        } else {
-            callback(new AJAXError(xhr.statusText, xhr.status, requestParameters.url));
-        }
-    };
-    xhr.send(payload);
-    return { cancel: () => xhr.abort() };
+export const postData = function(requestParameters: RequestParameters, callback: ResponseCallback<string>): Cancelable {
+    return makeRequest(extend(requestParameters, { method: 'POST' }), callback);
 };
 
 function sameOrigin(url) {
@@ -152,21 +129,21 @@ const transparentPngUrl = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAA
 export const getImage = function(requestParameters: RequestParameters, callback: Callback<HTMLImageElement>): Cancelable {
     // request the image with XHR to work around caching issues
     // see https://github.com/mapbox/mapbox-gl-js/issues/1470
-    return getArrayBuffer(requestParameters, (err, imgData) => {
+    return getArrayBuffer(requestParameters, (err: ?Error, data: ?ArrayBuffer, cacheControl: ?string, expires: ?string) => {
         if (err) {
             callback(err);
-        } else if (imgData) {
+        } else if (data) {
             const img: HTMLImageElement = new window.Image();
             const URL = window.URL || window.webkitURL;
             img.onload = () => {
                 callback(null, img);
                 URL.revokeObjectURL(img.src);
             };
             img.onerror = () => callback(new Error('Could not load image. Please make sure to use a supported image type such as PNG or JPEG. Note that SVGs are not supported.'));
-            const blob: Blob = new window.Blob([new Uint8Array(imgData.data)], { type: 'image/png' });
-            (img: any).cacheControl = imgData.cacheControl;
-            (img: any).expires = imgData.expires;
-            img.src = imgData.data.byteLength ? URL.createObjectURL(blob) : transparentPngUrl;
+            const blob: Blob = new window.Blob([new Uint8Array(data)], { type: 'image/png' });
+            (img: any).cacheControl = cacheControl;
+            (img: any).expires = expires;
+            img.src = data.byteLength ? URL.createObjectURL(blob) : transparentPngUrl;
         }
     });
 };

diff --git a/src/util/mapbox.js b/src/util/mapbox.js
@@ -199,25 +199,25 @@ export class TurnstileEvent {
             return this.processRequests();
         }
 
-        const evenstUrlObject: UrlObject = parseUrl(config.EVENTS_URL);
-        evenstUrlObject.params.push(`access_token=${config.ACCESS_TOKEN || ''}`);
+        const eventsUrlObject: UrlObject = parseUrl(config.EVENTS_URL);
+        eventsUrlObject.params.push(`access_token=${config.ACCESS_TOKEN || ''}`);
+
         const request: RequestParameters = {
-            url: formatUrl(evenstUrlObject),
+            url: formatUrl(eventsUrlObject),
             headers: {
-                'Content-Type': 'text/plain' //Skip the pre-flight OPTIONS request
-            }
+                'Content-Type': 'text/plain' // Skip the pre-flight OPTIONS request
+            },
+            body: JSON.stringify([{
+                event: 'appUserTurnstile',
+                created: (new Date(nextUpdate)).toISOString(),
+                sdkIdentifier: 'mapbox-gl-js',
+                sdkVersion: version,
+                'enabled.telemetry': false,
+                userId: this.eventData.anonId
+            }])
         };
 
-        const payload = JSON.stringify([{
-            event: 'appUserTurnstile',
-            created: (new Date(nextUpdate)).toISOString(),
-            sdkIdentifier: 'mapbox-gl-js',
-            sdkVersion: version,
-            'enabled.telemetry': false,
-            userId: this.eventData.anonId
-        }]);
-
-        this.pendingRequest = postData(request, payload, (error) => {
+        this.pendingRequest = postData(request, (error: ?Error) => {
             this.pendingRequest = null;
             if (!error) {
                 this.eventData.lastSuccess = nextUpdate;

diff --git a/test/ajax_stubs.js b/test/ajax_stubs.js
@@ -56,19 +56,19 @@ export const getArrayBuffer = function({ url }, callback) {
     if (cache[url]) return cached(cache[url], callback);
     return request({ url, encoding: null }, (error, response, body) => {
         if (!error && response.statusCode >= 200 && response.statusCode < 300) {
-            cache[url] = {data: body};
-            callback(null, {data: body});
+            cache[url] = body;
+            callback(null, body);
         } else {
             if (!error) error = { status: +response.statusCode };
             callback(error);
         }
     });
 };
 
-export const postData = function({ url }, payload, callback) {
-    return request.post(url, payload, (error, response, body) => {
+export const postData = function({ url, body }, callback) {
+    return request.post(url, body, (error, response, body) => {
         if (!error && response.statusCode >= 200 && response.statusCode < 300) {
-            callback(null, {data: body});
+            callback(null, body);
         } else {
             callback(error || new Error(response.statusCode));
         }

diff --git a/test/unit/util/ajax.test.js b/test/unit/util/ajax.test.js
@@ -17,18 +17,6 @@ test('ajax', (t) => {
         callback();
     });
 
-    t.test('getArrayBuffer, no content error', (t) => {
-        window.server.respondWith(request => {
-            request.respond(200, {'Content-Type': 'image/png'}, '');
-        });
-        getArrayBuffer({ url:'' }, (error) => {
-            t.pass('called getArrayBuffer');
-            t.ok(error, 'should error when the status is 200 without content.');
-            t.end();
-        });
-        window.server.respond();
-    });
-
     t.test('getArrayBuffer, 404', (t) => {
         window.server.respondWith(request => {
             request.respond(404);
@@ -102,7 +90,7 @@ test('ajax', (t) => {
         window.server.respondWith(request => {
             request.respond(204);
         });
-        postData({ url:'api.mapbox.com' }, {}, (error) => {
+        postData({ url:'api.mapbox.com' }, (error) => {
             t.equal(error, null);
             t.end();
         });