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
base: master
from

Conversation

3 participants
@DougReeder
Copy link
Contributor

DougReeder commented Oct 28, 2018

The primary need for this is running inside a Service Worker, where XHR is unavailable. It's also of use in Node, when a fetch() implementation is preferred over an XHR.

Secondarily, it allows timed-out requests to be aborted at the network level, so they don't continue to consume resources.

@galfert
Copy link
Member

galfert left a comment

First of all, thanks for this PR.

I haven't tested this with any apps yet, but the code looks pretty good.

Just left a question and a comment in the code itself.

case 'text':
return response.text();
default: // document
throw new Error("responseType 'document' is not currently supported using fetch");

This comment has been minimized.

@galfert

galfert Nov 2, 2018

Member

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

This comment has been minimized.

@DougReeder

DougReeder Nov 3, 2018

Contributor

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.

.gitignore Outdated
@@ -9,3 +9,5 @@ server-state
doc/code
doc/_build
/remotestorage*.js
.idea/
remotestorage.js.iml

This comment has been minimized.

@galfert

galfert Nov 2, 2018

Member

These files are specific to one's development environment. So they should rather be put in a global ignore file in your home directory (~/.gitignore). This way they are ignored for all projects you are working on.

This comment has been minimized.

@DougReeder

DougReeder Nov 3, 2018

Contributor

My second commit reverts this.

Show resolved Hide resolved src/wireclient.js

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

This comment has been minimized.

@galfert

galfert Nov 5, 2018

Member

What is this new change for?

This comment has been minimized.

@DougReeder

DougReeder Nov 6, 2018

Contributor

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

@DougReeder

This comment has been minimized.

Copy link
Contributor

DougReeder commented Nov 15, 2018

Are there any other concerns?

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Nov 15, 2018

I'll try to find a cycle for reviewing this tomorrow. We need to release some fixes asap anyway.

Sorry for the long wait!

@skddc
Copy link
Member

skddc left a comment

All looks great! 👍

I only found an issue with the timeout promise.


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

This comment has been minimized.

@skddc

skddc Nov 17, 2018

Member

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

This comment has been minimized.

@DougReeder

DougReeder Nov 18, 2018

Contributor

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.

@DougReeder DougReeder closed this Nov 18, 2018

@DougReeder

This comment has been minimized.

Copy link
Contributor

DougReeder commented Nov 18, 2018

closed in favor of #1140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment