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

Scaffold Core - Combine Electron, React, Edge, C# #1

Closed
tjcouch-sil opened this issue Jan 23, 2023 · 9 comments
Closed

Scaffold Core - Combine Electron, React, Edge, C# #1

tjcouch-sil opened this issue Jan 23, 2023 · 9 comments
Assignees

Comments

@tjcouch-sil
Copy link
Member

tjcouch-sil commented Jan 23, 2023

As a developer, I want to develop on a base of code projects including the relevant technologies for the software so I can develop the software.

  • Combine ERB and Edge
  • Add C# solution that integrates well with the Node stuff
@tjcouch-sil tjcouch-sil converted this from a draft issue Jan 23, 2023
@tjcouch-sil tjcouch-sil moved this from 🔖 Open to 🏗 In progress in Paranext Jan 23, 2023
@tjcouch-sil
Copy link
Member Author

tjcouch-sil commented Jan 23, 2023

@tjcouch-sil
Copy link
Member Author

tjcouch-sil commented Jan 23, 2023

It seems very challenging to get electron-edge-js to work.

We had to set some csproj settings to match the quick start project so it would include many C# dlls and the Edge dll provided in node_modules.

We discovered that electron-react-boilerplate has particular challenges with native module dependencies as detailed here:
https://electron-react-boilerplate.js.org/docs/adding-dependencies/#module-structure
https://www.electronjs.org/docs/latest/tutorial/using-native-node-modules
https://webpack.js.org/configuration/externals/
So we had to add electron-edge-js as a dependency in release/app/package.json instead. Then its postinstall runs @electron/rebuild to rebuild the native dependency for the current version of Electron.

@electron/rebuild was saying it couldn't get the abi for Electron 22.0.0. We had to uninstall and reinstall @electron/rebuild to rebuild electron-edge-js as a native dependency. If this does not work in the future, try adding a new node-abi as a dependency override:
electron/rebuild#1031
https://stackoverflow.com/questions/15806152/how-do-i-override-nested-npm-dependency-versions

Then node-gyp was saying it couldn't find Python. So I installed 3.6.x as detailed in the Building on Windows section of edge-js readme. choco install python (may need to add --version=3.6.8 if latest doesn't work)

Then it said it couldn't find a Visual Studio installation to use, so I used the Visual Studio Installer to install "Desktop development with C++". After that, npm install in release/app seemed to work and build a version of electron-edge-js.

Now I can see when debugging edge.js that it is finding the built version\release\app\node_modules\electron-edge-js\build\Release\edge_coreclr.node, but it still gives the error Error invoking remote method 'electronAPI.edge.invoke': TypeError: edge.initializeClrFunc is not a function when trying to run edge.func.

@irahopkinson
Copy link
Contributor

You probably already know this and this may not be related to the issue above but electron-edge-js has to have a matching electron and nodejs version. Thats why the main branch has electron v20 and the .nvmrc file specifies node v16.15. If you update the electron version you should also update the nodejs version (and the .nvmrc file) you are using to the one specified by electron-edge-js.

@tjcouch-sil
Copy link
Member Author

tjcouch-sil commented Jan 24, 2023

Thanks for the tip, Ira! Wasn't sure whether that would be an issue or not, but I think that makes sense now that I think about it more. Building a native dependency for Electron to run when Electron is running a different version of Node to run the native dependency probably wouldn't work well. Anyway, we uninstalled node and installed windows nvm choco install nvm. Then installed node 16.17.1 as per Electron 22.0.x's node version (we updated and tried 22.0.0 and 22.0.3 to match the quick start example repo and to be up-to-date). We continued getting the initializeClrFunc issue, so we checked the EDGE_APP_ROOT environment variable. It seemed to be pointing to the wrong place, so we changed it in main.ts to grab hopefully the right directory, and now it's giving us a different error. Now, electron-edge-js/lib/edge.js line 52 is actually throwing an exception instead of silently failing and returning an empty object, which as creating the original issue. We probably need to do more troubleshooting with the actual C# project now as it seems hopefully the edge native dependency is now working properly. The exception follows:
Error: Call to coreclr_create_delegate() for G failed with a return code of 0x80070002

Image

Edit: Solved by fixing the incorrect path to the EdgeJs.dll in EdgeLibrary.csproj according to this post!

I imagine we will have to work through getting it to work packaged and with asar and electron-builder and such at some point. Looks like you have to specify your built dll's as externalFiles or externalResources or something and possibly dotnet publish them. See the following links for more info:
agracio/edge-js-quick-start#3
https://github.com/agracio/electron-edge-js
agracio/electron-edge-js-quick-start@master...zenb:electron-edge-js-quick-start:master - example setting up electron-edge-js and electron-builder
https://github.com/bill-long/EventLogExpert/tree/master/Electron - example setting up electron-edge-js, electron-builder, and Angular
agracio/electron-edge-js#21 - mentions dotnet publish MyLibrary.csproj -r win10-x64 --self-contained

Here's another example repo of setting up asar unpacks dependent on the platform for .NET projects https://github.com/yoDon/electron-dotnet/blob/master/package.json

@tjcouch-sil
Copy link
Member Author

Now we're having an exception in production in edge or somewhere in .NET: Error: Error invoking remote method 'electronAPI.edge.invoke': System.TypeLoadException: Could not load type 'System.Object' from assembly 'System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e' because the parent does not exist. We don't know how to debug in the main.ts in production, but this does not happen in development.

Run npm run package:debug to get production builds.

agracio/electron-edge-js#21 has some words on publishing the C# project for Edge. The following links are the only places I could find anywhere that mention this error:
agracio/electron-edge-js#131
RicoSuter/NSwag#918
https://stackoverflow.com/questions/73324458/system-typeloadexception-could-not-load-type-system-object-from-assembly-sys
https://learn.microsoft.com/en-us/answers/questions/972245/is-it-possible-to-create-a-standalone-console-appl

@tjcouch-sil
Copy link
Member Author

Out of curiosity, I ran 10000 edge requests in the electron-edge-js-quick-start which uses nodeIntegration (heavily warned against by the electron devs) to allow you to call edge straight from the renderer instead of going through the electron ipc. The average time was ~60ms, and the total time was ~110ms. That's compared to going through electron ipc and edge where we saw the average time was ~550ms and the total time was ~1100ms. Compared to going through websocket to C# server to node websocket client and back where the average time was ~650ms and total time ~1140ms.

@tjcouch-sil
Copy link
Member Author

In considering whether setting nodeIntegration on is a good idea (because we're already running extension code in Node anyway... But the shim idea is in hopes that we can sandbox it at least somewhat. Or maybe just make a windowless renderer process with sandboxing?), I did some research on VSCode's settings.

The VSCode Sandboxing process article provided a lot of insight on how VS Code is structured. They have had nodeIntegration on for a long time, but they are moving toward sandboxing. See VSCode's BrowserWindow options for more info.

Notes from the article:

  • They use a windowless browser process for some operations like file watching and heavy computations to free up the main process from being weighed down and slowing the renderer
  • They use a UtilityProcess to run the extension host. It is unclear what the difference is between launching a utility process and using node's child_process library other than using Chromium instead of Node to fork the process. Maybe node processes don't support MessagePorts but UtilityProcesses do? Not sure if we gain any security from using these. Needs further investigation.
    • Note: VS Code's Extension host is one process per window, not one process per extension: "There is one extension host process per window and extensions are free to spawn as many child processes as they require"
  • They use MessagePorts to communicate directly between processes that need to communicate instead of always passing communication through the main process

VSCode's code organization: https://github.com/microsoft/vscode/wiki/source-code-organization

Following are more links pertaining to nodeIntegration:
https://stackoverflow.com/questions/57505082/would-it-be-safe-to-enable-nodeintegration-in-electron-on-a-local-page-that-is-p
https://www.electronjs.org/docs/latest/tutorial/security#17-validate-the-sender-of-all-ipc-messages

My takeaways:

  • It appears VSCode seems to have the posture that they think sandboxing is the right move, and this move they did not initially make is costing them heavily. They know a lot more about Electron security than we do, so they probably know what they're doing. Plus, our userbase is significantly less computer-literate than theirs, which makes me think we should at least try to implement the same security measures they do.
  • nodeIntegration on doesn't solve our packaging problems with Edge, so we would gain much less if we don't get Edge working. The isolation may be worth adding the time taken for ipc.
  • MessagePorts could make things somewhat faster, but they would tie us down to an Electron architecture and would require more work to get to a server/client architecture in the future. Probably worth reflecting more

@tjcouch-sil
Copy link
Member Author

tjcouch-sil commented Feb 1, 2023

We have decided to abandon Edge. There were 18 commits, and they compare to main here: main...09aab3f
The PR here was abandoned: #5

We will move forward with #16 and websockets

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Paranext Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
3 participants