-
-
Notifications
You must be signed in to change notification settings - Fork 956
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
Fix Electron throwing HTTP trailers are not supported
error
#598
Conversation
return is.function(value) ? value.bind(target) : value; | ||
} | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just this?
if (options.useElectronNet) {
response.trailers = [];
response.rawTrailers = [];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because electron set throwing getters for these. We need to override the getters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. You could use Object.defineProperty
, but I guess this is safer.
source/request-as-event-emitter.js
Outdated
@@ -47,13 +47,26 @@ module.exports = options => { | |||
|
|||
/* istanbul ignore next: electron.net is broken */ | |||
if (options.useElectronNet && process.versions.electron) { | |||
const electron = global['require']('electron'); // eslint-disable-line dot-notation | |||
const electron = require('electron'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was like this for a reason. It's to prevent Webpack from complaining. I guess we should add a comment about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK why but the "global" approach was throwing an error for me. I'll try that again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I get:
(node:8174) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: global.require is not a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how about this then:
const r = ({x: require})['yx'.slice(1)];
const electron = r('electron');
I would be impressed if Webpack managed to decipher that 😝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Smart :P
source/request-as-event-emitter.js
Outdated
fn = electron.net || electron.remote.net; | ||
} | ||
|
||
let timings; | ||
const cacheableRequest = new CacheableRequest(fn.request, options.cache); | ||
const cacheReq = cacheableRequest(options, response => { | ||
if (options.useElectronNet) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment with a link to the Electron issue this fixes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll do.
source/request-as-event-emitter.js
Outdated
@@ -1,4 +1,6 @@ | |||
'use strict'; | |||
/* istanbul ignore next: webpack only */ | |||
const r = ({x: require})['yx'.slice(1)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only used in one place, so just put it inline with the function call at line 50.
HTTP trailers are not supported
errorHTTP trailers are not supported
error
Can you fix the merge conflict? |
Fixes #461