-
-
Notifications
You must be signed in to change notification settings - Fork 297
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
XMLHttpRequest support #56
Comments
The more functionality the better! Are you sure the default request library doesnt support XMLHttpRequest ? Not sure if this is helpful: request/request#1267 If we did make the request library optional, we would have abtract out the request calls correct? Most of that is already done, inside the RequestHelper class, but im not sure if the other library supports all the options we use now? |
Unfortunately the resolution for that issue, "say in the headers that we used XHR", doesn't do anything for us, since the issue isn't convincing the server, it's that we are an electron app and we literally want to use electron's XHR implementation. All proxy/certificate settings are shared among all electron apps, so this means that if they can smoothly browse their gitlab instance (or gitlab.com) in Chrome, then they're already set up for all electron apps, including ours. I'm going to branch off |
Ah, I understand. Yea makes sense!
…On Apr 2, 2018, 1:46 PM, at 1:46 PM, Jordan Wallet ***@***.***> wrote:
Unfortunately the resolution for that issue, "say in the headers that
we used XHR", doesn't do anything for us, since the issue is that we
are an electron app and we literally want to use electron's XHR
implementation. All proxy/certificate settings are shared among all
electron apps, so this means that if they can smoothly browse their
gitlab instance (or gitlab.com) in Chrome, then they're already set up
for all electron apps, including ours.
I'm going to branch off `rc.8`, if that seems good to you. I'm thinking
this should use `Request` by default, but if something is constructed
with an explicit `useXMLHttpRequest: true` option, all requests will
assume that XMLHttpRequest is available in the current environment and
go through that.
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#56 (comment)
|
We faced a similar situation in electron apps where some requests prefer XHR, and others (need streaming) require the node const http_adapter = require('axios/lib/adapters/http');
// by default, axios detects the global window object and use XHR adapter
// http_adapter interfaces node.http
function login_fetch(url, params) { // XHR get, with auth headers
return axios({method: 'GET', headers: {"Authorization": "Bearer " + token}, params: params, url: url});
}
function login_post(url, data) { // XHR post, with auth headers
return axios({method: 'POST', headers: {"Authorization": "Bearer " + token}, data: data, url: url});
}
function fetch(url, params) { // very simple XHR get
return axios({method: 'GET', params: params, url: url});
}
function post_multipart_form(url, form) { // multipart form for file uploading, using adapter for node http
return axios.post(url, form, {headers: form.getHeaders(), adapter: http_adapter});
} Disclaimer: I have no connection with axios development. Just pass on the experience. |
This is great for electron apps (or basically any situation where you know the code is running in a browser, because you can exploit the browser's proxy resolution and SSL certificates. It's not normally available in Node, however, so it would have to be an option.
We swapped to this package and a few of our users are complaining about failures with self-signed certificates.
Basically this would be a constructor option that switches out the core driver of the requests. We did this on the original repository with a mix of PRs and fork-fixes for its
node-slumber
dependency. I totally expect to write this myself but I wanted to open the issue in advance to make sure you're amenable to such an extension to this package.The text was updated successfully, but these errors were encountered: