From a5b6f1b24321f1f34b5d975c5b3f07f9ed380db7 Mon Sep 17 00:00:00 2001 From: nodkz Date: Tue, 14 Mar 2017 22:39:01 +0600 Subject: [PATCH] fix: Bunch of small fixes, refactoring, cleanups --- README.md | 4 +- src/createRequestError.js | 77 ++++++++++++++++++ ...fetchWrapper.js => fetchWithMiddleware.js} | 59 ++++++++------ src/formatRequestErrors.js | 33 -------- src/middleware/batch.js | 81 ++++++++++++------- src/middleware/url.js | 4 +- src/relayMutation.js | 56 +++++-------- src/relayNetworkLayer.js | 8 +- src/relayQueries.js | 49 +++-------- 9 files changed, 200 insertions(+), 171 deletions(-) create mode 100644 src/createRequestError.js rename src/{fetchWrapper.js => fetchWithMiddleware.js} (51%) delete mode 100644 src/formatRequestErrors.js diff --git a/README.md b/README.md index d7b5c61..dab01ad 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ ReactRelayNetworkLayer ====================== [![](https://img.shields.io/npm/v/react-relay-network-layer.svg)](https://www.npmjs.com/package/react-relay-network-layer) -[![npm](https://img.shields.io/npm/dt/react-relay-network-layer.svg)](https://www.npmjs.com/package/react-relay-network-layer) +[![npm](https://img.shields.io/npm/dt/react-relay-network-layer.svg)](http://www.npmtrends.com/react-relay-network-layer) [![Travis](https://img.shields.io/travis/nodkz/react-relay-network-layer.svg?maxAge=2592000)](https://travis-ci.org/nodkz/react-relay-network-layer) [![Commitizen friendly](https://img.shields.io/badge/commitizen-friendly-brightgreen.svg)](http://commitizen.github.io/cz-cli/) [![semantic-release](https://img.shields.io/badge/%20%20%F0%9F%93%A6%F0%9F%9A%80-semantic--release-e10079.svg)](https://github.com/semantic-release/semantic-release) @@ -43,7 +43,7 @@ Previous documentation for version 1.x.x can be found [here](https://github.com/ - `batchUrl` - string or function(requestMap). Url of the server endpoint for batch request execution (default: `/graphql/batch`) - `batchTimeout` - integer in milliseconds, period of time for gathering multiple requests before sending them to the server (default: `0`) - `allowMutations` - by default batching disabled for mutations, you may enable it passing `true` (default: `false`) - - `maxBatchSize` - integer representing maximum size of request to be sent in a single batch. Once a request hits the provided size in length a new batch request is ran. (default: 102400 (roughly 100kb)) + - `maxBatchSize` - integer representing maximum size of request to be sent in a single batch. Once a request hits the provided size in length a new batch request is ran. (default: `102400` characters, roughly 100kb for 1-byte characters or 200kb for 2-byte characters) - **retryMiddleware** - for request retry if the initial request fails. - `fetchTimeout` - number in milliseconds that defines in how much time will request timeout after it has been sent to the server (default: `15000`). - `retryDelays` - array of millisecond that defines the values on which retries are based on (default: `[1000, 3000]`). diff --git a/src/createRequestError.js b/src/createRequestError.js new file mode 100644 index 0000000..352a222 --- /dev/null +++ b/src/createRequestError.js @@ -0,0 +1,77 @@ +/** + * Formats an error response from GraphQL server request. + */ +function formatRequestErrors(request, errors) { + const CONTEXT_BEFORE = 20; + const CONTEXT_LENGTH = 60; + + if (!request.getQueryString) { + return errors.join('\n'); + } + + const queryLines = request.getQueryString().split('\n'); + return errors + .map(({ locations, message }, ii) => { + const prefix = `${ii + 1}. `; + const indent = ' '.repeat(prefix.length); + + // custom errors thrown in graphql-server may not have locations + const locationMessage = locations + ? '\n' + + locations + .map(({ column, line }) => { + const queryLine = queryLines[line - 1]; + const offset = Math.min(column - 1, CONTEXT_BEFORE); + return [ + queryLine.substr(column - 1 - offset, CONTEXT_LENGTH), + `${' '.repeat(Math.max(offset, 0))}^^^`, + ] + .map(messageLine => indent + messageLine) + .join('\n'); + }) + .join('\n') + : ''; + return prefix + message + locationMessage; + }) + .join('\n'); +} + +export default function createRequestError( + request, + requestType, + responseStatus, + payload +) { + let errorReason; + if (typeof payload === 'object' && payload.errors) { + errorReason = formatRequestErrors(request, payload.errors); + } else if (payload) { + errorReason = payload; + } else { + errorReason = `Server response had an error status: ${responseStatus}`; + } + + let error; + if (request.getDebugName) { + // for native Relay request + error = new Error( + `Server request for ${requestType} \`${request.getDebugName()}\` ` + + `failed for the following reasons:\n\n${errorReason}` + ); + } else if (request && typeof request === 'object') { + // for batch request + const ids = Object.keys(request); + error = new Error( + `Server request for \`BATCH_QUERY:${ids.join(':')}\` ` + + `failed for the following reasons:\n\n${errorReason}` + ); + } else { + error = new Error( + `Server request failed for the following reasons:\n\n${errorReason}` + ); + } + + error.source = payload; + error.status = responseStatus; + return error; +} diff --git a/src/fetchWrapper.js b/src/fetchWithMiddleware.js similarity index 51% rename from src/fetchWrapper.js rename to src/fetchWithMiddleware.js index 1c8e6ea..55b8337 100644 --- a/src/fetchWrapper.js +++ b/src/fetchWithMiddleware.js @@ -1,6 +1,8 @@ /* eslint-disable no-param-reassign */ -export default function fetchWrapper(reqFromRelay, middlewares) { +import createRequestError from './createRequestError'; + +export default function fetchWithMiddleware(reqFromRelay, middlewares) { const fetchAfterAllWrappers = req => { let { url, ...opts } = req; @@ -12,27 +14,46 @@ export default function fetchWrapper(reqFromRelay, middlewares) { } } - return fetch(url, opts).then(res => { - // sub-promise for combining `res` with parsed json - return res - .json() - .then(json => { - res.json = json; + return fetch(url, opts) + .then(res => { + if (res.status >= 200 && res.status < 300) { return res; - }) - .catch(e => { - console.warn('error parsing response json', e); // eslint-disable-line no-console - res.json = {}; + } + return res.text().then(text => { + const err = new Error(text); + err.fetchResponse = res; + throw err; + }); + }) + .then(res => { + // sub-promise for combining `res` with parsed json + return res.json().then(json => { + res.json = json; return res; }); - }); + }); }; const wrappedFetch = compose(...middlewares)(fetchAfterAllWrappers); - return wrappedFetch(reqFromRelay) - .then(throwOnServerError) - .then(res => res.json); + return wrappedFetch(reqFromRelay).then(res => { + const payload = res.json; + if ({}.hasOwnProperty.call(payload, 'errors')) { + throw createRequestError( + reqFromRelay.relayReqObj, + reqFromRelay.relayReqType, + '200', + payload + ); + } else if (!{}.hasOwnProperty.call(payload, 'data')) { + throw new Error( + 'Server response.data was missing for query `' + + reqFromRelay.relayReqObj.getDebugName() + + '`.' + ); + } + return payload.data; + }); } /** @@ -55,11 +76,3 @@ function compose(...funcs) { rest.reduceRight((composed, f) => f(composed), last(...args)); } } - -function throwOnServerError(response) { - if (response.status >= 200 && response.status < 300) { - return response; - } - - throw response; -} diff --git a/src/formatRequestErrors.js b/src/formatRequestErrors.js deleted file mode 100644 index 9a485b4..0000000 --- a/src/formatRequestErrors.js +++ /dev/null @@ -1,33 +0,0 @@ -/** - * Formats an error response from GraphQL server request. - */ -export default function formatRequestErrors(request, errors) { - const CONTEXT_BEFORE = 20; - const CONTEXT_LENGTH = 60; - - const queryLines = request.getQueryString().split('\n'); - return errors - .map(({ locations, message }, ii) => { - const prefix = `${ii + 1}. `; - const indent = ' '.repeat(prefix.length); - - // custom errors thrown in graphql-server may not have locations - const locationMessage = locations - ? '\n' + - locations - .map(({ column, line }) => { - const queryLine = queryLines[line - 1]; - const offset = Math.min(column - 1, CONTEXT_BEFORE); - return [ - queryLine.substr(column - 1 - offset, CONTEXT_LENGTH), - `${' '.repeat(Math.max(offset, 0))}^^^`, - ] - .map(messageLine => indent + messageLine) - .join('\n'); - }) - .join('\n') - : ''; - return prefix + message + locationMessage; - }) - .join('\n'); -} diff --git a/src/middleware/batch.js b/src/middleware/batch.js index 5dcdcc7..75e8703 100644 --- a/src/middleware/batch.js +++ b/src/middleware/batch.js @@ -5,16 +5,6 @@ import { isFunction } from '../utils'; // Max out at roughly 100kb (express-graphql imposed max) const DEFAULT_BATCH_SIZE = 102400; -function copyBatchResponse(batchResponse, res) { - // Supports graphql-js, graphql-graphene and apollo-server responses - const json = res.payload ? res.payload : res; - return { - ok: batchResponse.ok, - status: batchResponse.status, - json, - }; -} - export default function batchMiddleware(opts = {}) { const batchTimeout = opts.batchTimeout || 0; // 0 is the same as nextTick in nodeJS const allowMutations = opts.allowMutations || false; @@ -24,7 +14,12 @@ export default function batchMiddleware(opts = {}) { return next => req => { - if (req.relayReqType === 'mutation' && !allowMutations) { + // never batch mutations with files + // mutation without files can be batched if allowMutations = true + if ( + req.relayReqType === 'mutation' && + (!allowMutations || (global.FormData && req.body instanceof FormData)) + ) { return next(req); } @@ -53,7 +48,17 @@ function passThroughBatch(req, next, opts) { // queue request return new Promise((resolve, reject) => { - singleton.batcher.requestMap[req.relayReqId] = { req, resolve, reject }; + singleton.batcher.requestMap[req.relayReqId] = { + req, + completeOk: res => { + req.done = true; + resolve(res); + }, + completeErr: err => { + req.done = true; + reject(err); + }, + }; }); } @@ -67,19 +72,9 @@ function prepareNewBatcher(next, opts) { setTimeout( () => { batcher.acceptRequests = false; - sendRequests(batcher.requestMap, next, opts).then(() => { - // check that server returns responses for all requests - Object.keys(batcher.requestMap).forEach(id => { - if (!batcher.requestMap[id].done) { - batcher.requestMap[id].reject( - new Error( - `Server does not return response for request with id ${id} \n` + - `eg. { "id": "${id}", "data": {} }` - ) - ); - } - }); - }); + sendRequests(batcher.requestMap, next, opts) + .then(() => finalizeUncompleted(batcher)) + .catch(() => finalizeUncompleted(batcher)); }, opts.batchTimeout ); @@ -95,8 +90,7 @@ function sendRequests(requestMap, next, opts) { const request = requestMap[ids[0]]; return next(request.req).then(res => { - request.done = true; - request.resolve(res); + request.completeOk(res); }); } else if (ids.length > 1) { // SEND AS BATCHED QUERY @@ -128,18 +122,43 @@ function sendRequests(requestMap, next, opts) { const request = requestMap[res.id]; if (request) { const responsePayload = copyBatchResponse(batchResponse, res); - request.done = true; - request.resolve(responsePayload); + request.completeOk(responsePayload); } }); }) .catch(e => { ids.forEach(id => { - requestMap[id].done = true; - requestMap[id].reject(e); + requestMap[id].completeErr(e); }); }); } return Promise.resolve(); } + +// check that server returns responses for all requests +function finalizeUncompleted(batcher) { + Object.keys(batcher.requestMap).forEach(id => { + if (!batcher.requestMap[id].req.done) { + batcher.requestMap[id].completeErr( + new Error( + `Server does not return response for request with id ${id} \n` + + `eg. { "id": "${id}", "data": {} }` + ) + ); + } + }); +} + +function copyBatchResponse(batchResponse, res) { + // Fallback for graphql-graphene and apollo-server batch responses + const json = res.payload || res; + return { + ok: batchResponse.ok, + status: batchResponse.status, + statusText: batchResponse.statusText, + url: batchResponse.url, + headers: batchResponse.headers, + json, + }; +} diff --git a/src/middleware/url.js b/src/middleware/url.js index a258238..98d33ea 100644 --- a/src/middleware/url.js +++ b/src/middleware/url.js @@ -16,7 +16,9 @@ export default function urlMiddleware(opts = {}) { } } - req.url = isFunction(urlOrThunk) ? urlOrThunk(req) : urlOrThunk; + if (req.relayReqType !== 'batch-query') { + req.url = isFunction(urlOrThunk) ? urlOrThunk(req) : urlOrThunk; + } return next(req); }; diff --git a/src/relayMutation.js b/src/relayMutation.js index 1387845..5dbf602 100644 --- a/src/relayMutation.js +++ b/src/relayMutation.js @@ -1,56 +1,41 @@ /* eslint-disable no-param-reassign, no-use-before-define, prefer-template */ -import formatRequestErrors from './formatRequestErrors'; - export default function mutation(relayRequest, fetchWithMiddleware) { const req = { method: 'POST', - relayReqId: Date.now(), + relayReqId: relayRequest.getID + ? relayRequest.getID() + : `mutation${Math.random()}`, relayReqObj: relayRequest, relayReqType: 'mutation', }; - if (_hasFiles(relayRequest)) { - Object.assign(req, _mutationWithFiles(relayRequest)); + if (hasFiles(relayRequest)) { + mutationWithFiles(req, relayRequest); } else { - Object.assign(req, _mutation(relayRequest)); + mutationWithoutFiles(req, relayRequest); } return fetchWithMiddleware(req) - .then(payload => { - if ({}.hasOwnProperty.call(payload, 'errors')) { - const error = new Error( - 'Server request for mutation `' + - relayRequest.getDebugName() + - '` ' + - 'failed for the following reasons:\n\n' + - formatRequestErrors(relayRequest, payload.errors) - ); - error.source = payload; - relayRequest.reject(error); - } else { - relayRequest.resolve({ response: payload.data }); - } - }) - .catch(error => relayRequest.reject(error)); + .then(data => relayRequest.resolve({ response: data })) + .catch(err => relayRequest.reject(err)); } -function _hasFiles(relayRequest) { +function hasFiles(relayRequest) { return !!(relayRequest.getFiles && relayRequest.getFiles()); } -function _mutationWithFiles(relayRequest) { - const req = { - headers: {}, - }; +function mutationWithFiles(req, relayRequest) { + req.headers = {}; - if (_hasFiles(relayRequest)) { + if (hasFiles(relayRequest)) { const files = relayRequest.getFiles(); if (!global.FormData) { throw new Error('Uploading files without `FormData` not supported.'); } const formData = new FormData(); + formData.append('id', req.relayReqId); formData.append('query', relayRequest.getQueryString()); formData.append('variables', JSON.stringify(relayRequest.getVariables())); Object.keys(files).forEach(filename => { @@ -64,22 +49,17 @@ function _mutationWithFiles(relayRequest) { }); req.body = formData; } - - return req; } -function _mutation(relayRequest) { - const req = { - headers: { - Accept: '*/*', - 'Content-Type': 'application/json', - }, +function mutationWithoutFiles(req, relayRequest) { + req.headers = { + Accept: '*/*', + 'Content-Type': 'application/json', }; req.body = JSON.stringify({ + id: req.relayReqId, query: relayRequest.getQueryString(), variables: relayRequest.getVariables(), }); - - return req; } diff --git a/src/relayNetworkLayer.js b/src/relayNetworkLayer.js index f2c0566..00a932c 100644 --- a/src/relayNetworkLayer.js +++ b/src/relayNetworkLayer.js @@ -1,6 +1,6 @@ import queries from './relayQueries'; import mutation from './relayMutation'; -import fetchWrapper from './fetchWrapper'; +import fetchWithMiddleware from './fetchWithMiddleware'; export default class RelayNetworkLayer { constructor(middlewares, options) { @@ -32,10 +32,12 @@ export default class RelayNetworkLayer { } sendQueries(requests) { - return queries(requests, req => fetchWrapper(req, this._middlewares)); + return queries(requests, req => + fetchWithMiddleware(req, this._middlewares)); } sendMutation(request) { - return mutation(request, req => fetchWrapper(req, this._middlewares)); + return mutation(request, req => + fetchWithMiddleware(req, this._middlewares)); } } diff --git a/src/relayQueries.js b/src/relayQueries.js index e7e7ca6..b1a6933 100644 --- a/src/relayQueries.js +++ b/src/relayQueries.js @@ -1,7 +1,5 @@ /* eslint-disable no-param-reassign, prefer-template */ -import formatRequestErrors from './formatRequestErrors'; - export default function queries(relayRequestList, fetchWithMiddleware) { return Promise.all( relayRequestList.map(relayRequest => { @@ -14,45 +12,16 @@ export default function queries(relayRequestList, fetchWithMiddleware) { Accept: '*/*', 'Content-Type': 'application/json', }, - body: prepareRequestBody(relayRequest), + body: JSON.stringify({ + id: relayRequest.getID(), + query: relayRequest.getQueryString(), + variables: relayRequest.getVariables(), + }), }; - return queryPost(relayRequest, fetchWithMiddleware(req)); - }) - ); -} -export function prepareRequestBody(relayRequest) { - return JSON.stringify({ - id: relayRequest.getID(), - query: relayRequest.getQueryString(), - variables: relayRequest.getVariables(), - }); -} - -export function queryPost(relayRequest, fetchPromise) { - return fetchPromise - .then(payload => { - if ({}.hasOwnProperty.call(payload, 'errors')) { - const error = new Error( - 'Server request for query `' + - relayRequest.getDebugName() + - '` ' + - 'failed for the following reasons:\n\n' + - formatRequestErrors(relayRequest, payload.errors) - ); - error.source = payload; - throw new Error(error); - } else if (!{}.hasOwnProperty.call(payload, 'data')) { - throw new Error( - 'Server response.data was missing for query `' + - relayRequest.getDebugName() + - '`.' - ); - } - return relayRequest.resolve({ response: payload.data }); + return fetchWithMiddleware(req) + .then(data => relayRequest.resolve({ response: data })) + .catch(err => relayRequest.reject(err)); }) - .catch(error => { - relayRequest.reject(error); - throw error; - }); + ); }