Support Electron 11 + other refactorings #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1) Upgrade to work with latest versions of all packages (including Electron 11). 2) Rely on ipc.config.appspace with app-name+PID to provide a unique namespace for the socketIds - so no need to check if the socket is in-use as it's guaranteed to be a dead socket if it's there. Also remove socket after use. This also makes it easier to pass around other socketIds - as long as they're unique within the app the appspace prefix will ensure they're unique across the entire system. 3) Pass IPC appspace and socketId as command line arguments to both renderer and process versions instead of passing them through electron IPC. This means you can connect immediately, simplifying init. (The `parseArgs` utility function is currently duplicated in the client and server - it's only 7 lines, and I couldn't figure out how to share code without adding a lot of JS tooling.) 4) As an example, also pass appVersion and isDev as through command line, which also makes it easy to remove the (unsafe) remote module in the renderer.
beorn
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi!
I really liked your approach in this example program - I think it's a really neat way to set up IPC between electron processes. I'm not sure if you're open for PRs for this, but as part of me going over your code and understanding your approach I thought of some ways to make it (I think) even better. I apologize that these are not split up into separate PRs - I figured that the code base is so small maybe it wouldn't be too hard to review together. If you don't agree with these changes that's fine too - I mostly did it for my own learnings anyways. That said, I really hope we can work together on best practices for Electron multi-process setups.
Here's a summary of the changes:
Upgraded to work with latest versions of all packages (including Electron 11).
Rely on ipc.config.appspace with app-name+PID to provide a unique namespace for the socketIds - so no need to check if the socket is in-use as it's guaranteed to be a dead socket if it's there. Also remove socket after use. This also makes it easier to pass around more socketIds - as long as they're unique within the app the appspace prefix will ensure they're unique across the entire system. (Also make the naming more consistent with the IPC module: socketName => socketId.)
Pass IPC appspace and socketId as command line arguments to both renderer and process versions instead of passing them through electron IPC and through CLI for subprocess version. I quite liked how this unified the code and approach for bootstrapping both the subprocess and renderer versions. This also means you can connect immediately, simplifying initialization. (The
parseArgsutility function is currently duplicated in the client and server - it's only 7 lines, and I couldn't figure out how to share code without adding a lot of JS tooling.)As an example, also pass appVersion and isDev as through command line, which also makes it easy to remove the (unsafe - and setting a bad example) remote module in the renderer.
Set up CSP for both HTML files.
I'd like to continue expanding on this to see how to support multiple services/servers, each with different webpack builds, perhaps more microservices type features such as discovery, and how to support observables across these connections.