search

sindresorhus / electron-better-ipc

Simplified IPC communication for Electron apps
MIT License
715 stars 60 forks source link

Fix Electron 12 compatibility #40

Closed dusansimic closed 3 years ago

dusansimic commented 3 years ago

This removes use of Electron remote so library would be compatible with Electron 12+. It's done by creating an invoke handler which will return window id so the answerMain listener can be crated for that specific window.

Maybe a better solution would be to rewrite the architecture and remove the use of window id all together. I'm not completely sure why it is done but to me it seems like when ipc call is sent to a browser window it's sent to that window only even if there is some other window listening on the same channel. Adding window id to the channel name look a little redundant. Is there some reason it's done that way or could it be removed and that would remove the requirement for getting the window id in the renderer process.

One big problem right now is the test-with-spectron test because I get an error when trying to run it on a newer version of electron and spectron.

1 test failed

  main

  /home/dusan/Dev/electron-better-ipc/node_modules/webdriver/build/utils.js:34

  Rejected promise returned by test. Reason:

  Error {
    message: `Failed to create session.␊
    Timeout awaiting 'request' for 10000ms`,
  }

  Object.startWebDriverSession (/home/dusan/Dev/electron-better-ipc/node_modules/webdriver/build/utils.js:34:15)
  Function.newSession (/home/dusan/Dev/electron-better-ipc/node_modules/webdriver/build/index.js:35:45)
  Object.exports.remote (/home/dusan/Dev/electron-better-ipc/node_modules/webdriverio/build/index.js:53:22)

It seems like webdriver can't connect to chromedriver for some odd reason and a request times out. This happens on newer version of electron (10+) but on older version I get some other odd errors and Spectron doesn't work either. I'm really not that experienced with Spectron and I did what research I could. I tried all proposed solutions from Spectron issues page and none worked. It would be good if I can get some feedback on this to see at least if it is platform specific (I run Linux).

sindresorhus commented 3 years ago

Maybe a better solution would be to rewrite the architecture and remove the use of window id all together. I'm not completely sure why it is done but to me it seems like when ipc call is sent to a browser window it's sent to that window only even if there is some other window listening on the same channel. Adding window id to the channel name look a little redundant. Is there some reason it's done that way or could it be removed and that would remove the requirement for getting the window id in the renderer process.

main.callRenderer needs to get the return value back from renderer.answerMain. Without the browser window ID, how would .callRenderer know which window it came from if main.callRendrer is emitted to multiple windows?

Does Electron have any way to get which window sent an IPC event? If not, may be worth an Electron issue for that. Sounds useful.

sindresorhus commented 3 years ago

I have no idea about the tests. Spectron has always been flaky...

dusansimic commented 3 years ago

I have no idea about the tests. Spectron has always been flaky...

I could make an electron app and try if everything works as expected and use that for testing from now on if that's okay with you. To be completely honest, it seems like the easiest option right now considering that test-with-spectron isn't being run in GitHub Actions anyway.

sindresorhus commented 3 years ago

Yeah, that's sounds fine. I ended up doing that in other projects.

https://github.com/sindresorhus/electron-store/blob/main/test.js

dusansimic commented 3 years ago

Does Electron have any way to get which window sent an IPC event? If not, may be worth an Electron issue for that. Sounds useful.

I think it does. If I understood you correctly a handler created by ipcMain needs to know from which window the call was sent. This can be done using the event object in the handler (link). In the event object there is sender object which is webContents that sent the call. If we know the id of the window that was called from main and the window that repsonded then one if statement would be sufficient to verify that. However then ipc.once couldn't be used and we would have to remove the listener manually.

sindresorhus commented 3 years ago

However then ipc.once couldn't be used and we would have to remove the listener manually.

That's fine as long as we're careful to do so.

dusansimic commented 3 years ago

Tests are written in a bit hacky way but they work. I don't particularly like how app is closed now (setInterval) but that is used to avoid closing app before getting all the logs from renderer process. Maybe we could set up webdriverjs or webdriverio for testing but it seems like much more effort than this while using basic Electron app also does the job.