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

Add electron-edge-js and C# project, update Electron to 22.0.3 and Node to 16.17.1 #5

Closed
wants to merge 18 commits into from

Conversation

tjcouch-sil
Copy link
Member

@tjcouch-sil tjcouch-sil commented Jan 24, 2023

  • Added nice eslint rules I developed from the React POC (we can always discuss code style preferences)
  • Added path aliases from the React POC for nice imports
  • Bumped nvmrc to 16.17.1
  • Bumped Electron to 22.0.3
  • Added electron-edge-js as a native dependency
  • Added EdgeLibrary.csproj (mostly from the edge POC) as a starting point for calling C# functions
  • Hooked up a Test Edge button that sends a message from the renderer to the main process, invokes an edge function, and returns a string that prints in the DevTools console

Resolves #1

Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all your work on this.

There are some files that were specifically removed from the boilerplate which I've added comments. You can't comment non-text files so also remove the 4x electron assets/icon.* files.

And I have one question about require in a TS file.

Also note there is a failing test in GH on MacOS.

.github/FUNDING.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/1-Bug_report.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/2-Question.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/3-Feature_request.md Outdated Show resolved Hide resolved
.github/config.yml Outdated Show resolved Hide resolved
.github/stale.yml Outdated Show resolved Hide resolved
.prettierrc Outdated Show resolved Hide resolved
src/main/main.ts Outdated
const baseNetAppPath = path.join(RESOURCES_PATH, '/c-sharp/bin/Debug/net7.0');
process.env.EDGE_USE_CORECLR = '1';
process.env.EDGE_APP_ROOT = baseNetAppPath;
const edge = require('electron-edge-js');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be required rather than imported? Linters don't like it, and I vaguely remember some packaging issues or something in the past.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edge needs some environment variables before import/require. EDGE_USE_CORECLR is easy to set in package.json in scripts, but EDGE_APP_ROOT points to the build location of the C# project, which I suppose may be different in different contexts. Would it be different if we were developing vs in packaged production? If not, maybe we can figure out putting EDGE_USE_CORECLR somewhere else. The reason I set environment variables here and then required edge instead of importing at the top is that it did the same in the quick start example, and I couldn't think of a better way for now since I don't really know what the project will look like packaged up and cross-platform and such. Are we going to use different build folders for different distributions? Will the folder be different between development and packaged? How do we address this issue in such a way that we can simply import edge at the top and the EDGE_APP_ROOT environment variable is set correctly? If you have thoughts, please share! Otherwise I will look at the mac deployment and see if that sheds any light. Thanks!

Copy link
Member Author

@tjcouch-sil tjcouch-sil Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the macos build job, the failure was in npm install:

Package mono-2 was not found in the pkg-config search path.
Perhaps you should add the directory containing `mono-2.pc'
to the PKG_CONFIG_PATH environment variable
No package 'mono-2' found
...
gyp: Call to 'pkg-config mono-2 --libs' returned exit status 1 while in binding.gyp. while trying to load binding.gyp
...
An unhandled error occurred inside electron-rebuild
node-gyp failed to rebuild '/Users/runner/work/paranext-core/paranext-core/release/app/node_modules/electron-edge-js'.
For more information, rerun with the DEBUG environment variable set to "electron-rebuild"

ERB is set up so that native module dependencies are

Looks like there are a number of dependencies that we will need to install in order to build electron-edge-js on the various platforms:

https://github.com/agracio/edge-js#building-on-windows
https://github.com/agracio/edge-js#building-on-osx
https://github.com/agracio/edge-js#building-on-linux

Alternatively, we could investigate using the prebuilt versions of the electron-edge-js package included in its package. We would want to figure out how to prevent .erb/scripts/electron-rebuild.js from firing electron-rebuild on electron-edge-js. However, I suspect this could hinder our cross-platform capabilities or future-proofing capabilities. I don't know, though, so maybe it's worth giving a shot! I will investigate. After all, it is rather annoying that people have to download all these dependencies to make electron-edge-js build on their machine. :/

@tjcouch-sil
Copy link
Member Author

Abandoning. See #1

@tjcouch-sil tjcouch-sil closed this Feb 1, 2023
@tjcouch-sil tjcouch-sil deleted the scaffold-core branch February 14, 2023 19:27
tjcouch-sil added a commit that referenced this pull request Jan 22, 2024
4cf4681b Fixed emotion package duplication (#6)
c44fb5ce Reworked explanation for package aliases. Also removed note about splitting into its own repo as this problem will not be solved as long as the package used is local
a3fa4976 Fixed emotion package duplication
f30019b4 Remove papi-components, replace with two libraries (#5)
b5d8fd1e remove papi-components, update packages, add platform-bible-utils as external

git-subtree-dir: extensions
git-subtree-split: 4cf4681b4ae4dbe1958d33dfc72d949021f7f6ae
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.

Scaffold Core - Combine Electron, React, Edge, C#
2 participants