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.

Sign up for GitHub

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

Jump to bottom

Support for electron-main process customization with DI #8076

Closed
wants to merge 4 commits into from
Closed

Support for electron-main process customization with DI #8076

wants to merge 4 commits into from

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Jun 23, 2020 •
edited by paul-marechal
Loading

What it does

This PR consists of the

Closes #7964
Closes #8069

This PR contributes a JSON-RPC communication scheme between the electron-main process and the renderer processes. It is reusing the same architecture as the websocket-based connection between frontend/backend, but adapted to work over electron's ipc apis. Currently there isn't scoped connections, meaning that each renderer process will interact with the same singleton in the electron-main process, but for the current use-cases this is sufficient.

How to test

Review checklist

Reminder for reviewers

@thegecko thegecko changed the title Support for elecrton-main process customization with DI Support for electron-main process customization with DI Jun 23, 2020
@vince-fugnitto vince-fugnitto added the electron issues related to the electron target label Jun 23, 2020
@paul-marechal paul-marechal force-pushed the electron-main-fixup branch 4 times, most recently from d71cef3 to 3c82aa0 Compare June 23, 2020 14:33
@paul-marechal
Copy link
Member

Thank you for the changes! I fixed nitpicks I found, but I'm happy with this PR.

Although I'm having trouble marking the Added dummy [...] commit as authored by yourself... Can you maybe do git commit --amend --reset-author and force push again? Sorry for the inconvenience. I remember I correctly fixed commit authorship before, but cannot now for some reason.

@kittaakos
Copy link
Contributor Author

Can you maybe do git commit --amend --reset-author and force push again? Sorry for the inconvenience.

I am fine with it, I do not really mind who's avatar is next to the commit. Is it a problem for you?

@akosyakov
Copy link
Member

Could you elaborate please in What it does: how different parts to talk to each other and how JSON-RPC multiplexing works in context of Electron, just to understand?

common means that electron APIs can be used, what electron-common means? Why it cannot just go to common?

akosyakov
akosyakov reviewed Jun 23, 2020
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
paul-marechal
paul-marechal approved these changes Jun 23, 2020
View reviewed changes
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Tested on Windows.

  • The Electron application works correctly.
  • A workspace is correctly opened when doing yarn start <path>.
  • The electron-main contributions are picked up.
  • I can see the traces for when we close a window and the IPC connection shutdowns.

packages/electron/src/electron-main/electron-application.ts Outdated Show resolved Hide resolved
@kittaakos
Copy link
Contributor Author

I fixed nitpicks I found,

Alright, what exactly? It would be nice to share this info with others. I thought you will review my part instead of modifying and force pushing it; so we can see the changes.

akosyakov
akosyakov previously approved these changes Jun 23, 2020
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

looking at sample looks quite clean :+1 adding a short PR description about new architecture would be helpful in the future

@akosyakov
Copy link
Member

Let's merge after the release, that's an arch change to merge in 3 days before the release.

paul-marechal
paul-marechal reviewed Jun 23, 2020
View reviewed changes
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

@kittaakos the nitpicks I had were so insignificant that I didn't want to just ask when I could just fix it. Sorry if you don't like me pushing to your branch, I will not do it again.

packages/electron/src/electron-main/electron-application.ts Outdated Show resolved Hide resolved
packages/electron/src/electron-main/electron-application.ts Outdated Show resolved Hide resolved
@paul-marechal paul-marechal force-pushed the electron-main-fixup branch 2 times, most recently from 2ff0ede to f875177 Compare June 23, 2020 15:10
@paul-marechal
Copy link
Member

looking at sample looks quite clean :+1 adding a short PR description about new architecture would be helpful in the future

You are entirely right, I added a simple description in packages/electron/README.md.

I also updated the PR description.

@paul-marechal
Copy link
Member

paul-marechal commented Jun 23, 2020 •
edited
Loading

common means that electron APIs can be used, what electron-common means? Why it cannot just go to common?

These are interfaces really specific to electron. Someone importing those interfaces from a browser script should really feel bad seeing something coming from electron-common, hopefully avoiding small mistakes.

@marcdumais-work
Copy link
Contributor

the nitpicks I had were so insignificant that I didn't want to just ask when I could just fix it. Sorry if you don't like me pushing to your branch, I will not do it again.

I just wanted to mention that GH has a feature that can help for such little "nitpicks", in a more transparent way: one can suggest changes, that the PR author can accept at the click of a button. See: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/incorporating-feedback-in-your-pull-request#applying-a-suggested-change

@akosyakov
Copy link
Member

akosyakov commented Jun 24, 2020 •
edited
Loading

Why did we decide to pull code in @theia/electron? How do you envision working on the same product supporting browser and electron without adding an explicit dependency to @theia/electron? It used to be a package to add native modules only to assemble the electron product.

@kittaakos
Copy link
Contributor Author

How do you envision working on the same product supporting browser and electron without adding an explicit dependency to @theia/electron?

As as a consumer of this change, I had the following understanding:

  • Let assume, you have an extension (my-ext-main) that works in both electron and web env, 👍
  • Your end product's package.json contains a bunch of @theia dependencies plus my-ext-main, we are still good 👍
  • Then you need something electron specific. You introduce a new my-ext-electron extension. This extension explicitly depends on @theia/electron and now you have to have a separate package.json for another product definition, and this package.json contains my-ext-main. The other does not.
  • This seems to be a good approach. Did I overlook something?

@akosyakov
Copy link
Member

akosyakov commented Jun 24, 2020 •
edited
Loading

This seems to be a good approach. Did I overlook something?

The original approach that the same extension can contribute entry point with overrides for Electron. It looks like this PR makes it impossible as well as breaking downstream extensions doing it. It is visible in the PR itself since window entry point was unnecessary moved to @theia/electron.

You introduce a new my-ext-electron extension.

that should not be necessary

@akosyakov akosyakov dismissed their stale review June 24, 2020 07:36

it needs more review for breaking changes

@kittaakos
Copy link
Contributor Author

It is visible in the PR itself since window entry point was unnecessary moved to @theia/electron.

That does not matter, from a downstream project's perspective, it's not a change. I never referenced the electron-specific window implementation, and I had to depend on @theia/electron anyway.

@akosyakov
Copy link
Member

Discussed with @kittaakos offline. It is indeed no-op for down streams, but there is a concern with how we divide code on extensions and js modules:

Introducing new extensions to separate by APIs goes against it. It would be good to revert changes to @theia/electron and merge new code into @theia/core and @theia/api-samples. If there is no any technical issues preventing it.

@kittaakos
Copy link
Contributor Author

It seems one of the force pushes broke the open new workspace command. No matter what I try to open, it does not open anything and I have the purplish status bar in the browser window. :/ I will fix it as part of the dev-package moving.

paul-marechal and others added 2 commits June 24, 2020 14:59
@paul-marechal
electron: use inversify in main process
1f234a1 
All the logic for the electron main process currently has to be added to
our generators, making it hard to extend without committing to Theia.

This commit re-arranges the way Electron is launched to allow people to
more easily change the behavior of their application.

Add a basic CLI to open a workspace by doing `app path/to/workspace`.

CLI can be overriden by application makers by extending and rebinding
`ElectronApplication.launch` and handling yourself the
`ExecutionParams`.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
Co-Authored-By: Akos Kitta <kittaakos@typefox.io>
Added a dummy electron-updater sample.
f443971 
Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@kittaakos kittaakos force-pushed the electron-main-fixup branch from f875177 to f443971 Compare June 24, 2020 13:35
@kittaakos
Copy link
Contributor Author

OK, I have moved back everything that originally was under dev-packages/electron, and moved the new things under packages/core. The new things are mainly under packages/core/src/electron-main. I also wiped the electron example extension, and moved the dummy updater to api-samples. I am aware of at least two issues:

  • when I refresh the browser window, the backend contributions are not disposed. I guess we changed something accidentally, and the WS connection reconnects. It should not.
  • when I start the app from a terminal and have opened at least two windows. I see an error when I terminate the app from the terminal (on macOS) with Ctrl+C.

I am looking into these now.

@kittaakos
Copy link
Contributor Author

kittaakos commented Jun 25, 2020 •
edited
Loading

To discuss:

fixed disposal when refreshig the browser window
0bf31fb 
Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@paul-marechal
Copy link
Member

Small heads up that I'll look into this again not before the coming monday.

This was referenced Jun 26, 2020
Merged
Closed
akosyakov
akosyakov reviewed Jun 29, 2020
View reviewed changes
examples/api-samples/package.json
"electronMain": "lib/electron-main/update/sample-updater-main-module"
},
{
"frontendElectron": "lib/electron-browser/updater/sample-updater-frontend-module"
Copy link
Member

Choose a reason for hiding this comment

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

It should belong to the previous entry point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, true. Thanks!

@akosyakov
Copy link
Member

My comments are minor, changes are looking good now. I have not tried though, waiting for the final version.

@gbodeen gbodeen mentioned this pull request Jun 30, 2020
Open
@paul-marechal
!fixup: fixes and cleanup
ce0ca5b 
Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@kittaakos kittaakos mentioned this pull request Jul 1, 2020
Closed
7 tasks
@kittaakos
Copy link
Contributor Author

Closing in favor of #8125

@kittaakos kittaakos closed this Jul 1, 2020
@kittaakos kittaakos deleted the electron-main-fixup branch August 18, 2020 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-marechal paul-marechal paul-marechal approved these changes

@akosyakov akosyakov akosyakov left review comments

@thegecko thegecko Awaiting requested review from thegecko

Assignees
No one assigned
Labels
electron issues related to the electron target
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kittaakos @paul-marechal @akosyakov @marcdumais-work @vince-fugnitto