Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicity set contextIsolation for Electron 12 #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bongnv
Copy link

@bongnv bongnv commented Oct 9, 2022

Description of the Change

Since Electron 12, the default of contextIsolation has changed to true from false so we'll have to set it to false explicitly. Otherwise, Node APIs won't be used in renderer processes. Example of error in the console:

[84267:1009/105137.986708:INFO:CONSOLE(7)] "Uncaught ReferenceError: require is not defined", source: file:///Users/bongnguyen/projects/pulsar/node_modules/github/lib/renderer.html?js=%2FUsers%2Fbongnguyen%2Fprojects%2Fpulsar%2Fnode_modules%2Fgithub%2Flib%2Fworker.js&managerWebContentsId=1&operationCountLimit=10&channelName=github%3Arenderer-ipc (7)
[84267:1009/105137.986816:INFO:CONSOLE(15)] "Uncaught ReferenceError: process is not defined", source: file:///Users/bongnguyen/projects/pulsar/node_modules/github/lib/renderer.html?js=%2FUsers%2Fbongnguyen%2Fprojects%2Fpulsar%2Fnode_modules%2Fgithub%2Flib%2Fworker.js&managerWebContentsId=1&operationCountLimit=10&channelName=github%3Arenderer-ipc (15)
[84267:1009/105137.996283:INFO:CONSOLE(7)] "Uncaught ReferenceError: require is not defined", source: file:///Users/bongnguyen/projects/pulsar/node_modules/github/lib/renderer.html?js=%2FUsers%2Fbongnguyen%2Fprojects%2Fpulsar%2Fnode_modules%2Fgithub%2Flib%2Fworker.js&managerWebContentsId=2&operationCountLimit=10&channelName=github%3Arenderer-ipc (7)
[84267:1009/105137.996381:INFO:CONSOLE(15)] "Uncaught ReferenceError: process is not defined", source: 

See electron/electron#23506 for more information.

Screenshot or Gif

N/A

Applicable Issues

N/A

@mauricioszabo
Copy link

This can't be right - I'm already using Electron 12 for a while, and we're not experiencing this problem.

If I'm not mistaken, contextIsolation doesn't even exist on newer electron versions, so I don't think we can rely on it anyway...

@Spiker985
Copy link
Member

contextIsolation = false is set in src/main-process/atom-window.js

I believe that I either tinkered with that setting while we were doing the initial rebrand, or it was already explicitly set

@Spiker985
Copy link
Member

Either way, we do need it for the time being. However, we should probably search for all instances of new BrowserWindow to make sure that it's explicitly specified where needed

@bongnv
Copy link
Author

bongnv commented Oct 9, 2022

This can't be right - I'm already using Electron 12 for a while, and we're not experiencing this problem.

If I'm not mistaken, contextIsolation doesn't even exist on newer electron versions, so I don't think we can rely on it anyway...

Did you try to open the Github window in the editor?
It's documented here: electron/electron#23506 that contextIsolation has been changed to true for the default value since Electron 12 so you should run into that problem too.

@bongnv
Copy link
Author

bongnv commented Oct 9, 2022

contextIsolation = false is set in src/main-process/atom-window.js

I believe that I either tinkered with that setting while we were doing the initial rebrand, or it was already explicitly set

Yes, you or @mauricioszabo did that for the editor when bumping to Electron 12: https://github.com/pulsar-edit/pulsar/pull/54/files#diff-23441b1e2ecd41b45a9b24a1f2bbf6af72c96396d34409560a5441813591179dR58

@mauricioszabo
Copy link

mauricioszabo commented Oct 9, 2022

Did you try to open the Github window in the editor

Yes, and it's working, why? What exactly are the problems you've been experiencing?

@bongnv
Copy link
Author

bongnv commented Oct 9, 2022

Did you try to open the Github window in the editor

Yes, and it's working, why? What exactly are the problems you've been experiencing?

I'm seeing it the log and I believe it's not working and the fix is needed. However, I'm not experiencing any visual bug. Considering, the code is from a worker, and I'm not familiar with the package, I'm unsure what kind of visual bug we'll see. Still, I'll leave PR here if anyone has any problem with Github package.

Regarding contextIsolation, I believe it should be removed from Electron 20 because they push forward to isolate the environments between main-process (node) and renderer(web). Pulsar will have to tackle this too but I believe the current fix is to set contextIsolation=true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants