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 master extension #4127

Closed

Conversation

bogthe
Copy link
Contributor

@bogthe bogthe commented Jan 21, 2019

Signed-off-by: Bogdan Stolojan petre.stolojan@arm.com

This PR fixes: #3364
By adding a new extension method called backendMasterElectron to modules which will be required into node inside of the master electron process and enabling packages like electron-updater to work with Theia.

@kittaakos @marechal-p Sorry I rushed and messed up the previous PR (#3431) so badly 😅 .

Reopening it here and addressing your previous comments. 🙇

Signed-off-by: Bogdan Stolojan <petre.stolojan@arm.com>
@bogthe bogthe force-pushed the electron-master-extension-pr branch from 4a2e014 to d0b657e Compare January 21, 2019 17:19
@bogthe
Copy link
Contributor Author

bogthe commented Jan 21, 2019

@kittaakos going back to naming, I used master because of isMaster check on the cluster.
How about electron-main-process?

@bogthe
Copy link
Contributor Author

bogthe commented Jan 21, 2019

@marechal-p, quoting your reply on the previous PR:

@bogthe I wasn't talking about the communication FE/BE, but rather the way they are constructed using inversify's container modules, which allows for contribution providers and symbol rebinding.

Currently the only way to extend the way the electron app part feels is by changing the generators, in the main repo. Another solution would be to forget about our generators and create your own. That could be different, that's my point.

I see, I think next it would be an easier / nice improvement to use inversify for this type of packages and have them in line with how the others work. If our requirements diverge more with theia's project roadmap implementing our own generators is definitely the way to go.

@akosyakov
Copy link
Member

@bogthe It was already decided to use inversify for customization. New entry points should be aligned with it.

//cc @svenefftinge

@svenefftinge
Copy link
Contributor

I agree with Anton, that we should keep it symmetric for the sake of simplicity. Does that work for you use case or would using inversify here as well cause any trouble?

@bogthe bogthe changed the title Adding files after foobar Electron master extension Jan 22, 2019
@bogthe
Copy link
Contributor Author

bogthe commented Jan 22, 2019

@svenefftinge I don't think it will cause any trouble, I'll have to look at how JsonRpcConnectionHandler and WebSocketConnectionProvider pass messages and probably add bit more changes to the generated files.

@bogthe bogthe force-pushed the electron-master-extension-pr branch from 1f4a9e1 to 6321e03 Compare January 23, 2019 09:12
bogthe and others added 4 commits January 23, 2019 09:46
Signed-off-by: Bogdan Stolojan <petre.stolojan@arm.com>
Signed-off-by: Bogdan Stolojan <petre.stolojan@arm.com>
Signed-off-by: Bogdan Stolojan <petre.stolojan@arm.com>
Signed-off-by: Bogdan Stolojan <petre.stolojan@arm.com>
@bogthe bogthe force-pushed the electron-master-extension-pr branch from 6321e03 to da883f5 Compare January 23, 2019 13:09
Bogdan Stolojan added 2 commits January 23, 2019 13:11
Signed-off-by: Bogdan Stolojan <petre.stolojan@arm.com>
Signed-off-by: Bogdan Stolojan <petre.stolojan@arm.com>
@bogthe bogthe force-pushed the electron-master-extension-pr branch from 5281816 to 1773b59 Compare January 23, 2019 13:55
@bogthe
Copy link
Contributor Author

bogthe commented Jan 23, 2019

Now using inversify for this entry point. Only thing needed is a way to trigger / access these modules.
@svenefftinge I would like some guidance around that if possible 😄 , what's your opinion on the following:

  • Have a thin wrapper around ipcMain, ipcRenderer that will allow messages to be passed between FE and BE. Preferred because it's relatively easy to implement and any FE package can use it;
  • Or have the same setup for these type of modules similar to back-end application, i.e. a server listening for at a specific port etc. Seems like a bit more work and integration with the code base.

@akosyakov
Copy link
Member

@bogthe just to clarify that we are on the same page

the process structure looks like:

  • frontend: electron main process -> electron renderer process
  • backend: electron main process -> (server master process ->)? server worker process

You want to hook into electron main process to run electron-updater there and be able to talk between electron main and electron renderer processes?

This PR provides an entry point for server master process, not for electron main. Are you sure it is what you need? Should not this PR provides an entry point to electron main process by instantiating a container within electron-main.js file?

After that you can create an extension which contributes to electron main and frontnend electron entry points and establish communication via ipcMain or ipcRenderer. electron renderer process (FE) can communicate already with server worker process (BE) over JSON-RPC

@vince-fugnitto vince-fugnitto added the electron issues related to the electron target label Jan 31, 2020
@kittaakos
Copy link
Contributor

I propose closing this PR in favor of #8076.

@westbury
Copy link
Contributor

I propose closing this PR in favor of #8076.

This PR was opened while @bogthe was working on Mbed Studio. I tried to build Mbed Studio with #8076 to test it out but it would have taken a lot of work. However this PR is clearly obsolete in view of #8076 so I'm closing this one.

@westbury westbury closed this Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Hook into electron main master process
6 participants