request: Replace deprecated request module with net.request.

[manavmehta]

Any background context you want to provide?
Fixes: #886

You have tested this PR on:

• macOS Catalina 10.15.5
• Windows
• Linux/Ubuntu

Checklist

• domain-util
• reconnect-util
• linuxupdater
• check add-new-server
• check UA on test server

We may think of a more appropriate and suitable name of the new added request.ts

[manavmehta]

Also, for the record, remote is now available as a package (on it's way to be moved out from electron core)

[andersk]

Let’s move discussion about remote to #915.

(But to answer your questions in short: yes, we should eliminate remote. Yes, we should eliminate it completely. No, there are no cases of it that should not be eliminated. It’s being moved from core to a package because it should not be used, not because it should be used. In case you haven’t read this page, which I linked from #915 and is also linked from the Electron bug and the @electron/remote documentation, see Electron’s ‘remote’ module considered harmful. I think we’re all on the same page here. Good? Good.)

[abhigyank]

The original functionality remains intact for me after the migration. Further, now we don't need certificate-util, I tested adding certificates to Chrome store and using certutilon Linux, and server gets added without a hitch.
All in all LGTM.
One minor issue, in case of certificate error on server addition, we show two dialog box errors - one from electron.net and one we create. But I think that's a bug in how we handle certificate errors, could be a follow-up PR.

[manavmehta]

Going with this approach, I think it will handle two dialog boxes.

[abhigyank]

Let's keep getProxy for now? I feel it completes the the utility of a file like proxy-util though unused. Could be wrong here.
Removing request-util is correct.

[andersk]

Disagree. There’s no point in keeping around code that won’t be used with electron.net. If it burns, set it on fire.

[abhigyank]

I agree it won't be used with electron.net, however I disagree that keeping it could be worse. I disagree also that we should fire to anything that burns, houses, cars everything burns, I don't suppose we should set fire to them. Just because anything burns, we shouldn't burn them.

[andersk]

Uh…unused code is not a house or a car. Unused code is a readability complication, a maintenance burden, a potential attack surface, and an all around waste of time and space. There are rare cases where unused code is useful for API symmetry or to help the type checker or something, but this isn’t one of them. This code should just go away.

[abhigyank]

There are rare cases where unused code is useful for API symmetry.

Yeah I was going for this, so that a util could have a set and get function. Since, we don't have it as an API explicitly, we could let it go then. One can always re-add it later if required. We can keep the commit that removes it :)

[andersk]

We don’t have a set function and a get function here. A get function should return exactly the data that you put into a set function—symmetry. This getProxy function, on the other hand, returns the result of some weird string-mangling logic and maybe also sets an environment variable as a side effect. Its only purpose was to support the specific library that we’re now removing.

[abhigyank]

@andersk Can you review this for merging?

[andersk]

Let’s try to extract the raw Promise logic into minimal shared helper functions and lean on async/await as much as possible. With a small fix to the Electron typings in typings.d.ts (justified by the documentation: “IncomingMessage implements the Readable Stream interface”):

declare module 'electron' {
    interface IncomingMessage extends NodeJS.ReadableStream {}
}

we can do this:

import getStream from 'get-stream';

async function fetchResponse(request: ClientRequest): Promise<IncomingMessage> {
    return new Promise((resolve, reject) => {
        request.on("response", resolve);
        request.on("abort", reject);
        request.on("error", reject);
        request.end();
    });
}

export async function _getServerSettings(…): Promise<ServerConf> {
    const response = await fetchResponse(net.request({
        url: domain + '/api/v1/server_settings',
        session,
    }));
    if (response.statusCode !== 200) {
        throw response;  // maybe we want this to be an actual Error?
    }
    const {realm_name, realm_uri, realm_icon} = JSON.parse(await getStream.buffer(response));
    …
}

[andersk]

Don’t capitalize the variable Session.

[andersk]

This also seems to trigger an extremely unfortunate bug where one of the renderer processes goes into a 100% CPU loop that eventually consumes all available memory. I’m trying to debug this…

[andersk]

The bug had something to do with trying to serialize an entire ReconnectUtil over the IPC channel. We only need its url, so yeah, let’s not do that.

[manavmehta]

Thanks @andersk. Though,

one of the renderer processes goes into a 100% CPU loop that eventually consumes all available memory.

I didn't get this totally. Can you please explain it to me?

[akashnimare]

Thanks, everyone 🤝