Fix bug in Electron environment. by mokeyish · Pull Request #17684 · emscripten-core/emscripten

mokeyish · 2022-08-19T13:09:28Z

sbc100 · 2022-08-19T22:04:19Z

Is seems like you don't want the require('fs').readFile approach to be used in the electron environment? Doesn't the fs module exist there?

If you want to disable node support you can also just specify -sENVIRONMENT=web.. then this code won't even be included (giving you a smaller app).

mokeyish · 2022-08-19T22:30:32Z

@sbc100 Hi, It's not a problem in build time.
I am using pyodide to enable my plugin to run python code block in electron based markdown editor (obsidian).
But it recognized eletron environment (no fs module) as node environment throw error.

I fix it temporay in pyodide.asm.js by adding typeof process.browser !== 'undefined'

sbc100 · 2022-08-19T22:43:23Z

@sbc100 Hi, It's not a problem in build time. I am using pyodide to enable my plugin to run python code block in electron based markdown editor (obsidian). But it recognized eletron environment (no fs module) as node environment throw error.

I fix it temporay in pyodide.asm.js by adding typeof process.browser !== 'undefined'

I was trying to say that if you set -sENVIRONMENT=web then the code that looks for fs module will not even be included the generated code.

Are you saying that fs module does not exist in electron? (Sorry I know nothing of electron).

mokeyish · 2022-08-19T22:48:35Z

@sbc100 Both node and browser are supported by pyodide. It can't just set -sENVIRONMENT=web.

Electron also has process，but it no nodejs environment actually.

sbc100 · 2022-08-19T23:12:58Z

@sbc100 Both node and browser are supported by pyodide. It can't just set -sENVIRONMENT=web.

Electron also has process，but it no nodejs environment actually.

And require('fs') doesn't work in electron?

mokeyish · 2022-08-19T23:39:42Z

@sbc100 Yes, it will thow error. electron is browser environment.

sbc100 · 2022-08-19T23:41:04Z

@sbc100 Yes, it will thow error. electron is browser environment.

But I thought one of the main points of electron was that you did have access to things like local files via node APIs? I must be mis-remembering?

mokeyish · 2022-08-19T23:47:07Z

@sbc100 Yes, it will thow error. electron is browser environment.

But I thought one of the main points of electron was that you did have access to things like local files via node APIs? I must be mis-remembering?

You can access local files via node APIs. But when process.browser=true, it should not use fs module.

mokeyish · 2022-08-19T23:50:06Z

So, I modified here.

sbc100 · 2022-08-19T23:50:48Z

    if options.support_node:
      node_support_code = '''
-        if (typeof process === 'object' && typeof process.versions === 'object' && typeof process.versions.node === 'string') {
+        if (typeof process === 'object' && typeof process.browser === 'undefined' && typeof process.versions === 'object' && typeof process.versions.node === 'string') {


Can we simplify this to just if (typeof process === 'object' && !process.browser && process.versions && process.versions.node).. just to save a few bytes? (i.e. the typeof is only needed for the first clause I think).

We have what a basically duplicates of this code in src/shell.js and src/worker.js. I guess they both should be updated and kept in sync with this one?

@sbc100 I have already updated src/shell.js andsrc/worker.js.

sbc100 · 2022-08-20T14:57:20Z

It would be nice to add a least some testing for the electron environment at some point. If you are using electric with emscripten perhaps you would be interesting in adding this? I'll open a bug.

curiousdannii · 2022-08-21T01:08:59Z

I can't see anything in the Electron docs about a process.browser property. Should this check process.type instead?

mokeyish · 2022-08-21T01:36:32Z

I can't see anything in the Electron docs about a process.browser property. Should this check process.type instead?

It used here.

https://github.com/pyodide/pyodide/blob/02644bd353f54719758d7b3b768b89a5cede319d/src/js/compat.ts#L3-L10

mokeyish · 2022-08-21T01:37:27Z

It would be nice to add a least some testing for the electron environment at some point. If you are using electric with emscripten perhaps you would be interesting in adding this? I'll open a bug.

I don't know how to make a testing for it.

curiousdannii · 2022-08-21T01:38:51Z

It would be better not to use undocumented APIs.

mokeyish · 2022-08-21T01:58:15Z

It would be better not to use undocumented APIs.

@curiousdannii It's just to judge whether to use Node APIs. process.browser is intuitive.

@hoodmane hello, what do you think. I see the code below was commited by you.
https://github.com/pyodide/pyodide/blob/02644bd353f54719758d7b3b768b89a5cede319d/src/js/compat.ts#L3-L10

mokeyish · 2022-08-21T02:00:51Z

sbc100 · 2022-08-23T04:52:09Z

It would be better not to use undocumented APIs.

Its not really an API, more of a way of detecting the environment in which the code is running. Do you know of an better or more official way to detect this mode of electron?

curiousdannii · 2022-08-23T05:28:19Z

@sbc100 As I suggested, process.type seems to be the official way? Since process.browser is undocumented there's a chance they could remove it.

mokeyish · 2022-08-23T05:38:42Z

@sbc100 As I suggested, process.type seems to be the official way? Since process.browser is undocumented there's a chance they could remove it.

process.type is a readonly property, can't be configured. (process#processtype-readonly,)

If use process.type === 'browser' pyodide will also throws error in electron environment, the value of process.type is renderer.

sbc100 · 2022-08-23T05:47:28Z

If use process.type === 'browser' pyodide will also throws error in electron environment, the value of process.type is renderer.

So we can check for process.type == 'renderer'? Sounds reasonable.

sbc100 · 2022-08-23T05:48:43Z

sbc100 · 2022-08-23T05:49:24Z

@sbc100 As I suggested, process.type seems to be the official way? Since process.browser is undocumented there's a chance they could remove it.

My apologies .. I missed that reply somehow.

curiousdannii · 2022-08-23T05:54:36Z

It will need proper testing (which I haven't done any of.) If "browser" is for the "main" process, ie, the Node process rather than the Chromium process, then it's very poorly named. "renderer" is probably for the Chromium process in which require won't work. It would also be good to check that web workers within Electron are being detected properly and the existence of process isn't tripping them up.

The code could also check for process.versions.chrome or process.versions.electron, but it's probably more important to use process.type.

mokeyish · 2022-08-23T06:13:48Z

@sbc100 As I suggested, process.type seems to be the official way? Since process.browser is undocumented there's a chance they could remove it.

@curiousdannii

process.browser is not an Api of electron.

process.browser is used here https://github.com/pyodide/pyodide/blob/02644bd353f54719758d7b3b768b89a5cede319d/src/js/compat.ts#L3-L10

Then, In order to use pyodide in electron, I defind process.browser = true here. 😂
https://github.com/mokeyish/obsidian-code-emitter/blob/49edc9cb3941f1463ab037a71865ebc92c1e4fbd/src/backend/languages/python.ts#L6-L8

curiousdannii · 2022-08-23T21:31:01Z

@mokeyish So you mean process.browser is only meant to be something that pyodide checks? This PR definitely needs to be updated in that case!

mokeyish · 2022-08-24T00:22:11Z

@mokeyish So you mean process.browser is only meant to be something that pyodide checks? This PR definitely needs to be updated in that case!

Yes， the way to check if IN_NODE is basically the same as emscripten, except process.browser.

hoodmane · 2022-08-24T00:54:52Z

We should probably add an option in loadPyodide to declare that the runtime is browser-like or node-like in cases that the detection doesn't work correctly.

sbc100 · 2022-08-24T01:55:28Z

Can't your IN_NODE just add && process.type != 'renderer and thereby avoid the whole process.browser thing?

Seems like we want to be checking process.type

Fix bug in Electron environment.

0553a04

sbc100 reviewed Aug 19, 2022

View reviewed changes

mokeyish added 4 commits August 20, 2022 07:55

Update file_packager.py

69f5202

Update shell.js

ec6dd38

Update worker.js

1d3d875

Update file_packager.py

330d959

sbc100 reviewed Aug 20, 2022

View reviewed changes

Comment thread tools/file_packager.py Outdated

sbc100 previously approved these changes Aug 20, 2022

View reviewed changes

sbc100 mentioned this pull request Aug 20, 2022

Add some testing of emscripten output under electron #17692

Open

sbc100 enabled auto-merge (squash) August 20, 2022 14:59

sbc100 closed this Aug 23, 2022

auto-merge was automatically disabled August 23, 2022 05:48
Pull request was closed

sbc100 reopened this Aug 23, 2022

This comment was marked as resolved.

Sign in to view

sbc100 mentioned this pull request Sep 25, 2024

Fix Electron renderer process compatability #22619

Merged

Uh oh!

Conversation

mokeyish commented Aug 19, 2022

Uh oh!

sbc100 commented Aug 19, 2022

Uh oh!

mokeyish commented Aug 19, 2022

Uh oh!

sbc100 commented Aug 19, 2022

Uh oh!

mokeyish commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokeyish commented Aug 19, 2022

Uh oh!

sbc100 commented Aug 19, 2022

Uh oh!

mokeyish commented Aug 19, 2022

Uh oh!

mokeyish commented Aug 19, 2022

Uh oh!

sbc100 Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

mokeyish Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

mokeyish Aug 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbc100 commented Aug 20, 2022

Uh oh!

curiousdannii commented Aug 21, 2022

Uh oh!

mokeyish commented Aug 21, 2022

Uh oh!

mokeyish commented Aug 21, 2022

Uh oh!

curiousdannii commented Aug 21, 2022

Uh oh!

mokeyish commented Aug 21, 2022

Uh oh!

mokeyish commented Aug 21, 2022

Uh oh!

sbc100 commented Aug 23, 2022

Uh oh!

curiousdannii commented Aug 23, 2022

Uh oh!

mokeyish commented Aug 23, 2022

Uh oh!

sbc100 commented Aug 23, 2022

Uh oh!

sbc100 commented Aug 23, 2022

Uh oh!

sbc100 commented Aug 23, 2022

Uh oh!

curiousdannii commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokeyish commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

curiousdannii commented Aug 23, 2022

Uh oh!

mokeyish commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoodmane commented Aug 24, 2022

Uh oh!

sbc100 commented Aug 24, 2022

Uh oh!

This comment was marked as resolved.

Reviewers

Assignees

Labels

Projects

mokeyish commented Aug 19, 2022 •

edited

Loading

sbc100 commented Aug 19, 2022 •

edited

Loading

curiousdannii commented Aug 23, 2022 •

edited

Loading

mokeyish commented Aug 23, 2022 •

edited

Loading

mokeyish commented Aug 24, 2022 •

edited

Loading