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

Electron package #165

Merged
merged 27 commits into from
Jul 27, 2021
Merged

Electron package #165

merged 27 commits into from
Jul 27, 2021

Conversation

johnb8
Copy link
Member

@johnb8 johnb8 commented Jun 28, 2021

Addresses #57. I couldn't implement it in the same way we do in our LC package since app-config doesn't have synchronous config loading anymore and Electron preload scripts don't wait for promises to resolve. Instead I load the config in the main electron process then pass that through to the renderer. I think this is better than the way we used to do it since the config is now available on the main electron side as well. I'm not sure if there's any concerns using the main config object across libraries like I do, but it works.

I'll add documentation and tests, just wanted to get your thoughts on how I've done this first.

Copy link
Contributor

@joelgallant joelgallant left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for working on this!

app-config-electron/src/index.ts Outdated Show resolved Hide resolved
app-config-electron/src/index.ts Outdated Show resolved Hide resolved
app-config-electron/src/index.ts Show resolved Hide resolved
app-config-electron/README.md Outdated Show resolved Hide resolved
app-config-electron/README.md Outdated Show resolved Hide resolved
app-config-electron/README.md Outdated Show resolved Hide resolved
app-config-electron/README.md Outdated Show resolved Hide resolved
app-config-electron/README.md Outdated Show resolved Hide resolved
app-config-electron/README.md Show resolved Hide resolved
app-config-electron/src/index.ts Show resolved Hide resolved
app-config-electron/package.json Show resolved Hide resolved
examples/electron-project/package.json Outdated Show resolved Hide resolved
app-config-electron/package.json Outdated Show resolved Hide resolved
johnb8 and others added 2 commits July 2, 2021 19:14
@johnb8 johnb8 force-pushed the electron-plugin branch from 8399fdb to db7d5ac Compare July 18, 2021 19:56
@joelgallant
Copy link
Contributor

@johnb8 seems to have timed out tests, not sure why

@johnb8
Copy link
Member Author

johnb8 commented Jul 23, 2021

Yeah, not really sure what's going on, it doesn't seem to be running the electron tests at all? I added the playwright github action, but since it's electron it should have all the dependencies it needs so I'm not sure if it will help. I'm also not sure if it's a display thing either because I think Windows and MacOS should run it no problem without any display configuration. @joelgallant can you approve the run again?

@johnb8
Copy link
Member Author

johnb8 commented Jul 25, 2021

Could you approve again @joelgallant?

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2021

Codecov Report

Merging #165 (7dc0548) into master (1792d22) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #165   +/-   ##
=======================================
  Coverage   80.09%   80.09%           
=======================================
  Files          42       42           
  Lines        2482     2482           
  Branches      593      593           
=======================================
  Hits         1988     1988           
  Misses        494      494           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1792d22...7dc0548. Read the comment docs.

@johnb8
Copy link
Member Author

johnb8 commented Jul 26, 2021

@joelgallant are the Windows and MacOS pipelines actually running in those environments? If you look at the yarn playwright install-deps chromium step on them it says it's installing the Ubuntu dependencies and the paths are all Linuxy

@johnb8
Copy link
Member Author

johnb8 commented Jul 27, 2021

@joelgallant CI is fixed, I think this is good to go now, unless you see anything else?

@johnb8 johnb8 linked an issue Jul 27, 2021 that may be closed by this pull request
Copy link
Contributor

@joelgallant joelgallant 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 the effort in getting this done!

.github/workflows/main.yml Outdated Show resolved Hide resolved
@johnb8 johnb8 merged commit c6e8c2a into launchcodedev:master Jul 27, 2021
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.

Add Electron package for injecting config into web apps
3 participants