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

Fix Electron throwing HTTP trailers are not supported error #598

Merged
merged 8 commits into from
Aug 31, 2018

Conversation

szmarczak
Copy link
Collaborator

Fixes #461

return is.function(value) ? value.bind(target) : value;
}
});
}
Copy link
Owner

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 = [];
}

Copy link
Collaborator Author

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.

Copy link
Owner

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.

@@ -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');
Copy link
Owner

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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

Copy link
Owner

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 😝

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... Smart :P

fn = electron.net || electron.remote.net;
}

let timings;
const cacheableRequest = new CacheableRequest(fn.request, options.cache);
const cacheReq = cacheableRequest(options, response => {
if (options.useElectronNet) {
Copy link
Owner

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'll do.

@@ -1,4 +1,6 @@
'use strict';
/* istanbul ignore next: webpack only */
const r = ({x: require})['yx'.slice(1)];
Copy link
Owner

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.

@sindresorhus sindresorhus changed the title Fix electron throwing HTTP trailers are not supported error Fix Electron throwing HTTP trailers are not supported error Aug 31, 2018
@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@sindresorhus sindresorhus merged commit e66a6b6 into sindresorhus:master Aug 31, 2018
@szmarczak szmarczak deleted the fix-electron branch January 17, 2019 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants