Fix Electron usage

Question

Fix Electron usage

vbourgeois opened this issue 5 years ago · 24 comments

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)

Answer 1 · 2019-10-23T10:47:27.000Z

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.

Answer 2 · 2019-11-12T11:11:32.000Z

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

Answer 3 · 2019-11-12T11:12:13.000Z

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 )

Answer 4 · 2019-11-12T15:03:06.000Z

@arantes555 thanks for the heads up :)

Answer 5 · 2019-11-15T15:42:42.000Z

@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.

Answer 6 · 2019-11-17T17:25:03.000Z

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.

Answer 7 · 2019-11-18T04:17:12.000Z

@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.

Answer 8 · 2019-11-18T09:15:29.000Z

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

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

Answer 9 · 2019-11-18T09:33:17.000Z

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

Answer 10 · 2019-11-18T22:54:31.000Z

electron/electron#21055

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

Answer 11 · 2019-11-18T22:56:11.000Z

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

Answer 12 · 2019-11-19T04:44:14.000Z

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

Answer 13 · 2019-12-10T16:04:55.000Z

Hello,

I tried to update to the last version of got (10.0.3), and I now have the following errors :

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

=>

GotError: The "listener" argument must be of type Function. Received type object
    at get (/home/vincent/lls/lls-desktop/node_modules/got/dist/source/request-as-event-emitter.js:199:27)
    at checkListener (events.js:63:11)
    at ClientRequest.once (events.js:304:3)
    at new ClientRequest (electron/js2c/browser_init.js:2422:12)
    at Object../lib/browser/api/net.js.Net.request (electron/js2c/browser_init.js:2621:10)
    at get (/home/vincent/lls/lls-desktop/node_modules/got/dist/source/request-as-event-emitter.js:196:43)
    at /home/vincent/lls/lls-desktop/node_modules/got/dist/source/request-as-event-emitter.js:260:13

And same error with :

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

Everything works fine if I remove the useElectronNet option.
Any hint on this ?

Thanks.

Answer 14 · 2019-12-10T16:18:08.000Z

@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.

Answer 15 · 2019-12-14T14:41:11.000Z

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

Answer 16 · 2019-12-14T15:41:16.000Z

@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.

Answer 17 · 2019-12-14T21:18:20.000Z

@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.

Answer 18 · 2019-12-16T11:14:04.000Z

@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.

Answer 19 · 2019-12-19T16:51:15.000Z

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 👍

Answer 20 · 2019-12-19T19:49:01.000Z

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

Answer 21 · 2019-12-23T21:09:39.000Z

Note: When the electron.net support is removed, you'll still be able to use Got in the Electron main process and in the renderer process through the electron.remote module or if you use Node.js shims.

Answer 22 · 2020-01-15T13:16:12.000Z

One question, as I just saw the thread, and Got is working fine for me — will only the net-module-support be dropped, or won't Got work altogether without these workarounds you mentioned? I don't use net, because Got does everything I need and works perfectly, so this would be the question …?

Answer 23 · 2020-01-15T15:44:12.000Z

Only the net support will be dropped.

Answer 24 · 2020-01-15T16:10:04.000Z

Perfect, thank you very much for the clarification!