Fix Electron usage

[vbourgeois]

Describe the bug

• Node.js version: 12.13.0
• Electron version : 7.0.0
• OS & version: linux mint 19.2

When using got in electron, useElectronNet option is not working with electron 7 for POST requests, returns net::ERR_INVALID_ARGUMENT error.

Actual behavior

HTTP request returns the following error :

GotError [RequestError]: net::ERR_INVALID_ARGUMENT
      at ClientRequest.<anonymous> (/home/vincent/lls/lls-desktop/node_modules/got/source/request-as-event-emitter.js:192:19)
      at Object.onceWrapper (events.js:291:20)
      at ClientRequest.emit (events.js:208:15)
      at ClientRequest.origin.emit (/home/vincent/lls/lls-desktop/node_modules/@szmarczak/http-timer/source/index.js:37:11)
      at URLRequest../lib/browser/api/net.js.URLRequest._emitRequestEvent (electron/js2c/browser_init.js:2284:24) {
    name: 'RequestError',
    host: 'jsonplaceholder.typicode.com',
    hostname: 'jsonplaceholder.typicode.com',
    method: 'POST',
    path: '/posts',
    socketPath: undefined,
    protocol: 'https:',
    url: 'https://jsonplaceholder.typicode.com/posts',
    gotOptions: {
      path: '/posts',
      protocol: 'https:',
      slashes: true,
      auth: null,
      host: 'jsonplaceholder.typicode.com',
      port: null,
      hostname: 'jsonplaceholder.typicode.com',
      hash: null,
      search: null,
      query: null,
      pathname: '/posts',
      href: 'https://jsonplaceholder.typicode.com/posts',
      retry: {
        retries: [Function],
        methods: [Set],
        statusCodes: [Set],
        errorCodes: [Set]
      },
      headers: {
        'user-agent': 'got/9.6.0 (https://github.com/sindresorhus/got)',
        accept: 'application/json',
        'accept-encoding': 'gzip, deflate',
        'content-type': 'application/json',
        'content-length': 39
      },
      hooks: {
        beforeRequest: [],
        beforeRedirect: [],
        beforeRetry: [],
        afterResponse: [],
        beforeError: [],
        init: []
      },
      decompress: true,
      throwHttpErrors: true,
      followRedirect: true,
      stream: false,
      form: false,
      json: true,
      cache: false,
      useElectronNet: true,
      body: '{"title":"foo","body":"bar","userId":1}',
      method: 'POST'
    }
  }

Expected behavior

Correct HTTP response

Code to reproduce

In electron main or renderer process :

got
    .post('https://jsonplaceholder.typicode.com/posts', {
      body: {
        title: 'foo',
        body: 'bar',
        userId: 1
      },
      json: true,
      useElectronNet: true
    })
    .then(console.log)
    .catch(console.log)

[vbourgeois]

For GET requests, I get the following error :

GotError [ReadError]: incorrect header check
      at EventEmitter.<anonymous> (/home/vincent/lls/lls-desktop/node_modules/got/source/as-promise.js:28:12)
      at processTicksAndRejections (internal/process/task_queues.js:85:5) {
    name: 'ReadError',
    code: 'Z_DATA_ERROR',
    host: 'www.google.fr',
    hostname: 'www.google.fr',
    method: 'GET',
    path: '/',
    socketPath: undefined,
    protocol: 'http:',
    url: 'http://www.google.fr/',
    gotOptions: {
      path: '/',
      protocol: 'http:',
      slashes: true,
      auth: null,
      host: 'www.google.fr',
      port: null,
      hostname: 'www.google.fr',
      hash: null,
      search: null,
      query: null,
      pathname: '/',
      href: 'http://www.google.fr/',
      retry: {
        retries: [Function],
        methods: [Set],
        statusCodes: [Set],
        errorCodes: [Set]
      },
      headers: {
        'user-agent': 'got/9.6.0 (https://github.com/sindresorhus/got)',
        'accept-encoding': 'gzip, deflate'
      },
      hooks: {
        beforeRequest: [],
        beforeRedirect: [],
        beforeRetry: [],
        afterResponse: [],
        beforeError: [],
        init: []
      },
      decompress: true,
      throwHttpErrors: true,
      followRedirect: true,
      stream: false,
      form: false,
      json: false,
      cache: false,
      useElectronNet: true,
      method: 'GET'
    }
  }

with

got
    .get('http://www.google.fr', {
      useElectronNet: true
    })
    .then(console.log)
    .catch(console.log)

It is working if I remove useElectronNet option.

[arantes555]

It is probably linked to a recent (undocumented) change in electron@7 where setting content-length explicitly fails : electron/electron#21091

[arantes555]

It is probably linked to a recent (undocumented) change in electron@7 where setting content-length explicitly fails : electron/electron#21091

(I have the same issue on electron-fetch arantes555/electron-fetch#17 )

[szmarczak]

@arantes555 thanks for the heads up :)

[arantes555]

@szmarczak it turns out, in this specific case, it is indeed a breaking change in electron's API but not a bug per-say. You just cannot set explicitly a content-length in electron >= 7, it is computed automatically in electron.

[szmarczak]

Ok, so I have made progress on this:

Things that do not work:

> OPTIONS
- encoding
- timeout (only `timeout.request` works)
- retry
- followRedirect
- maxRedirects
- decompress
- dnsCache
- agent
- hooks.beforeRedirect
- hooks.beforeRetry
- timings - working improperly

> RESPONSE
- response.redirectUrls

Things that can be fixed:

> OPTIONS
- retry
- hooks.beforeRedirect
- hooks.beforeRetry

> RESPONSE
- response.redirectUrls

TODO (applies only when using electron):

• rename options.session to options.electronSession
• make options.redirect constant (manual to support beforeRedirect hook)
• send auth info using request.once('login', ...)
• options.path = options.url.pathname + options.url.search
• make tests using avaron

You can see my work here.

[sindresorhus]

@szmarczak Honestly, I would prefer us opening an Electron issue about these differences instead of littering the codebase with workarounds. Electron says it's Node.js compatible, so it's up to them to fix it.

[arantes555]

@sindresorhus @szmarczak I believe there are PRs in electron that will fix most of these issues :

• fix: webRequest should be able to modify CORS headers electron/electron#21099
• fix: allow reading body from non-2xx responses in net.request electron/electron#21055

I suggest waiting until these are merged, and electron@7.2 is released with these fixes.

[sindresorhus]

And for anything not fixed by those PRs or doesn't have an existing issue, we should open new Electron issues for.

[szmarczak]

electron/electron#21055

Haven't electron had this already? :OOOOOOOOOOOOO (not enough Os here)

[arantes555]

@szmarczak they had it, they broke it ^^ electron@7 broke a lot of things in net...

[sindresorhus]

Every single Electron release breaks a lot of random things...

[szmarczak]

@vbourgeois We are aware that electron support is broken as of now - it's because electron is not compatible with the Node.js net API. We'll provide more info when we get it working.

[szmarczak]

I have made more progress:
✔️ response.redirectUrls is not empty anymore
✔️ options.session was renamed to options.electronSession
✔️ beforeRedirect hooks work as expected
✔️ option.followRedirect works as expected
✔️ option.responseType works as expected

✖️ options.retry does not work (yet)
✖️ options.timeout has no effect (only timeout.request works)
✖️ options.decompress has no effect
✖️ options.dnsCache has no effect
✖️ Agent is not supported
✖️ timings are broken (won't fix)

will send a PR in 10 mins

[sindresorhus]

@szmarczak At this point, is it really worth all the effort? Electron will continue to break things and be incompatible. I feel like this is a losing battle. Electron support was only added because the Electron team said the net API was compatible. It's clearly not and it doesn't look like it ever will be. I honestly think it would be better to just mark the Electron support as broken and deprecated and remove it in the next major version.

[szmarczak]

@sindresorhus I agree. Sent a PR - it will also display a deprecation warning. I think we should drop Electron support in a minor release, as it's been broken since Got v9. Or we can just drop it now.

[sindresorhus]

@szmarczak I don't think we should drop it completely as it might work for people in some scenarios, and dropping it would be a breaking change. However, what we can do is to remove the docs for the Electron option and mention of Electron support, except the FAQ thing. And then open an issue about removing it completely in Got v11.

[MarshallOfSound]

Hey @sindresorhus

This got sent my way, just wanted to check in here RE in net module in Electron. In particular this bit from your comment.

Electron support was only added because the Electron team said the net API was compatible.

I haven't checked the history of our docs but our current net documentation doesn't claim API compatibility, the closest it gets is this.

The API components (including classes, methods, properties and event names) are similar to those used in Node.js.

We made the API close so that the super simple "request a thing from URL(X)" work across http and net, anything complicated will fall over pretty quickly.

I think dropping support here is the right move as they are fundamentally not API compatible and trying to make it look to users like they are will just result in weird bugs / issues for all involved. There are also lots of options to http.request like agent and dnsCache that Electron can just never support in net due to the way Chromiums networking stack is structured.

If there's a part of our docs that I've missed that does erroneously claim compatibility let me know and I will gladly scrub it or reword it 👍

[sindresorhus]

@MarshallOfSound It was removed here: electron/electron@8ea33d6#diff-4193de697f9ec0a7a4b858cb1a6ea968L24-L25 by my request: electron/electron#8117 (comment)

[sindresorhus]

Only the net support will be dropped.

[nathanlesage]

Perfect, thank you very much for the clarification!