Fix Electron renderer process compatability - #22619
Conversation
| // N.b. Electron.js environment is simultaneously a NODE-environment, but | ||
| // also a web environment. | ||
| var ENVIRONMENT_IS_NODE = typeof process == 'object' && typeof process.versions == 'object' && typeof process.versions.node == 'string'; | ||
| var ENVIRONMENT_IS_NODE = typeof process == 'object' && typeof process.versions == 'object' && typeof process.versions.node == 'string' && process.type != 'renderer'; |
There was a problem hiding this comment.
Does electron set process.versions.node? It looks like the check for process.versions.node was added specifically to deal with electron.
There was a problem hiding this comment.
Does electron set
process.versions.node?
Yes
There was a problem hiding this comment.
Was that always the case?
It seems that the processs.versions.node check here was explicitly added to distinguish between node and electron: e9971d1.
In any case it seems like you don't need both the process.versions.node check and the process.type check?
There was a problem hiding this comment.
https://www.electronjs.org/docs/latest/tutorial/electron-timelines
Electron v22.0.0 comes with Node.js as v16.17.
Electron v22.0.0 was released in 2022, while e9971d1 is for 2019.
There was a problem hiding this comment.
The process comes from the contextBridge.exposeInMainWorld('process', process) in the preload script.
There was a problem hiding this comment.
So its possible to expose the process module in some way.. but process.type will always be renderer?
In that case this LGTM, but can you drop the process.versions.node check?
Again I wish we had some kind of testing for electron in our CI :(
There was a problem hiding this comment.
So its possible to expose the
processmodule in some way.. butprocess.typewill always berenderer?
Yes
This is the result under Electron v32
There was a problem hiding this comment.
In that case this LGTM, but can you drop the
process.versions.nodecheck?
Feels like it's possible, but not sure it'll work on a version that's too old.
There was a problem hiding this comment.
Actually I think we do still want the process.versions.node check, so that if process is defined by some other means its doesn't trigger this to be true.
|
Looks like this change replaces #17684 |
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
lgtm.
You can rebaseline the codesize tests using test/runner other.*code_size* other.*codesize* --rebaseline
OK |
sbc100
changed the title
|
Oh, I forget to say, its best to do |
|
Hi, I don't really understand the purpose of this PR, but at the moment, this little change |
3 participants
https://www.electronjs.org/docs/latest/api/process#processtype-readonly