Bolster isBrowser check for electron-link bundled applications

[themadtitanmathos]

#436

[EzzhevNikita]

@themadtitanmathos Could you please explain why did you change the package name?

[themadtitanmathos]

@themadtitanmathos Could you please explain why did you change the package name?

Whoops, that was from our fork we are using in the meantime while we were waiting for review. I was sharing the same branch for both PRs, and completely forgot about that.

Will undo that right now!

[anatolybolshakov]

Could it make sense to add null check for windows.navigator property instead of introducing electron specific logic?

[Dobby007]

@anatolybolshakov it is not enough. Please refer to this discussion: electron/electron#2288. Although they are recommending to check userAgent....

[Dobby007]

I believe that the correct way for doing this is to just keep the first check
const isElectron: boolean = process && process.versions && process.versions.hasOwnProperty('electron');
but remove the latter one
const isElectronWebpage: boolean = isElectron && (<ElectronProcess><unknown>process).type === 'renderer';

because if it is the renderer navigator.userAgent will be defined.

[Mr-Wallet]

Is there a way we can get this PR un-stuck? We'd rather not maintain our own fork of this package indefinitely. 😅

A userAgent check for a specific value wouldn't work for us, as we override the userAgent on the default session so that it doesn't have electron in it. (Long story.) However it is truthy, so if it's simply a check for the existence of navigator and of the userAgent thereof, that would work.

However that doesn't seem to be what's discussed in the electron issue that was linked; that issue was more about trying to determine if a browser-like process is electron or not; and vaguely to me it sounds like, "if context isolation is enabled/access to Node is disabled, and the userAgent isn't roughly default, there's no way to tell." But frankly at that point, we don't actually need to tell; if it's browser-like, isBrowser === true will work regardless of whether or not it's Electron.

The main issue of this PR is making sure it doesn't break when it's not like a normal browser (i.e. there is no window.navigator). For both normal browsers and Electron renderers (regardless of how isolated or not they are), the isBrowser path in this file is definitely safe. We just need to know when it's not safe so that we can avoid it, and unfortunately window (the existing check) is defined in Electron 'main' processes, but there's also no window.navigator (which is the actual dependency in the isBrowser code path).

For us it seemed sufficient to do what's in this PR; if a renderer is isolated it might false-negative and think it's not Electron when it is; but in that case we wanted isBrowser to be true anyway, so we get the right answer regardless. Context isolation (which can hide the fact that it's Electron) is impossible for the 'main' process, which is the case we're trying to solve for. For our purposes, this PR gets the job done, and with regards to 'renderer' processes (which are the focus of electron/electron#2288), I don't think this can break any use case (isolation or no). Isolated 'worker' processes are unsolved but this package wouldn't work in such a process anyway; and we haven't tested it but un-isolated workers would probably be fixed under this change as well.

This PR seems strictly the same or better than the existing code for all use cases, but we're not picky as long as we can get something merged. Please let me know your thoughts.

(P.S. unrelated but please bump typed-rest-client to 1.8.8 or higher)

[anatolybolshakov]

@themadtitanmathos thanks for contribution! Could you please take a look at the comments?

[Dobby007]

@anatolybolshakov I just stumbled over the same issue. I don't know why using of userAgent was introduced but it's probably important for Azure DevOps API. A fix is desperately needed for this.