Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wireclient uses fetch() if available, instead of XHR #1132

Closed
wants to merge 13 commits into from
Closed
4 changes: 2 additions & 2 deletions src/util.js
Expand Up @@ -35,10 +35,10 @@ var util = {
}
},

globalContext: (typeof(window) !== 'undefined' ? window : global),
globalContext: (typeof(window) !== 'undefined' ? window : (typeof self === 'object' ? self : global)),

getGlobalContext () {
return (typeof(window) !== 'undefined' ? window : global);
return (typeof(window) !== 'undefined' ? window : (typeof self === 'object' ? self : global));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this new change for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sets the global context, when run in a Service Worker, where the global variable is called 'self'.

},

extend (target) {
Expand Down
79 changes: 76 additions & 3 deletions src/wireclient.js
Expand Up @@ -7,7 +7,7 @@ const Authorize = require('./authorize');
const config = require('./config');

/**
* This file exposes a get/put/delete interface on top of XMLHttpRequest.
* This file exposes a get/put/delete interface on top of fetch() or XMLHttpRequest.
* It requires to be configured with parameters about the remotestorage server to
* connect to.
* Each instance of WireClient is always associated with a single remotestorage
Expand Down Expand Up @@ -490,8 +490,81 @@ WireClient.prototype = {
WireClient.isArrayBufferView = isArrayBufferView;

// Shared request function used by WireClient, GoogleDrive and Dropbox.
// TODO: Should we use fetch ?
WireClient.request = function (method, url, options) {
if (typeof fetch === 'function') {
return WireClient._fetchRequest(method, url, options);
} else if (typeof XMLHttpRequest === 'function') {
return WireClient._xhrRequest(method, url, options);
} else {
log('[WireClient] no support for HTTP requests');
return Promise.reject('[WireClient] no support for HTTP requests');
}
};

/** options includes body, headers and responseType */
WireClient._fetchRequest = function (method, url, options) {
var syntheticXhr;
var responseHeaders = {};
var abortController;
if (typeof AbortController === 'function') {
abortController = new AbortController();
}
var networkPromise = fetch(url, {
method: method,
headers: options.headers,
body: options.body,
signal: abortController ? abortController.signal : undefined
}).then(function (response) {
log('[WireClient fetch]', response);

response.headers.forEach(function (value, headerName) {
responseHeaders[headerName.toUpperCase()] = value;
});

syntheticXhr = {
readyState: 4,
status: response.status,
statusText: response.statusText,
response: undefined,
getResponseHeader: function (headerName) {
return responseHeaders[headerName.toUpperCase()] || null;
},
// responseText: 'foo',
responseType: options.responseType,
responseURL: url,
};
switch (options.responseType) {
case 'arraybuffer':
return response.arrayBuffer();
case 'blob':
return response.blob();
case 'json':
return response.json();
case '':
case 'text':
return response.text();
default: // document
throw new Error("responseType 'document' is not currently supported using fetch");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would this be triggered? For HTML files with the appropriate mime type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is controlled by the calling function, which sets options.responseType. It is independent of the Content-type returned by the server.

So far as I can tell, all current code always sets options.responseType to 'arraybuffer' and converts as needed, outside WireClient.request. However, WireClient.request is a public interface, and 'blob', 'json' and 'text' are plausible values (and easily implemented).

'document' is used by XHR to produce an HTML Document or XML XMLDocument. It is possible to write code to produce those from the fetch response, but I can't forsee any use for that. That would be something UI code would use.

}
}).then(function (processedBody) {
syntheticXhr.response = processedBody;
return syntheticXhr;
});

var timeoutPromise = new Promise(function (resolve, reject) {
setTimeout(function () {
log('[WireClient] request timed out', url);
Copy link
Member

@raucao raucao Nov 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this promise is raced, but nothing is cancelled, this code will still be run. I'm not sure about potential side effects (for example I had rs-backup not exit the program for me after syncing all items with this branch, but not sure if related). However, the line is always logged in the debug logs, which is definitely not a good idea:

screenshot from 2018-11-17 11-16-24

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! That log() line should be removed.

Unfortunately, I think I rebased at the wrong time before pushing the commit with this. I'll make a new branch and PR with just my changes.

reject('timeout');
if (abortController) {
abortController.abort();
}
}, config.requestTimeout);
});
DougReeder marked this conversation as resolved.
Show resolved Hide resolved

return Promise.race([networkPromise, timeoutPromise]);
};

WireClient._xhrRequest = function (method, url, options) {
return new Promise ((resolve, reject) => {

log('[WireClient]', method, url);
Expand Down Expand Up @@ -554,7 +627,7 @@ WireClient._rs_init = function (remoteStorage) {
};

WireClient._rs_supported = function () {
return !! XMLHttpRequest;
return typeof fetch === 'function' || typeof XMLHttpRequest === 'function';
};

WireClient._rs_cleanup = function () {
Expand Down