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

Replace dependency on deprecated electron-remote package #152

Open
mlalkaka opened this issue Jun 15, 2019 · 2 comments
Open

Replace dependency on deprecated electron-remote package #152

mlalkaka opened this issue Jun 15, 2019 · 2 comments

Comments

@mlalkaka
Copy link
Contributor

Currently, electron-spellchecker uses electron-remote for some of its functionality. Though electron-remote is deprecated, this still worked. However, in Electron 5, node integration has been turned off by default (electron #4362), which is something that electron-remote implicitly relied on (#148).

I didn't realize this when I first submitted PR #149, so although that PR gets electron-spellchecker working in Electron 5 on MacOS, it still doesn't work on Windows and Linux, hence #151 😞.

I think there are two options for resolving this problem:

  1. Fork the deprecated electron-remote package, make the change to turn on Node integration in the BrowserWindows it creates, and make electron-spellchecker use that. If this is the last thing needed to get electron-spellchecker working in Electron 5, then this is the fastest path to Electron 5 support.
  2. Replace electron-remote with things that work in Electron 5. Given that electron-remote is deprecated anyway, this is probably the better long-term solution.

I created this issue to track option 2 above. From what I can tell, there are 3 places where electron-remote is used in electron-spellchecker. I've been doing some research into how to replace these instances with something else. Here's what I've found.

  1. Downloading Hunspell dictionaries (dictionary-sync.js). I think it might be better to use electron-dl for this. Unfortunately, electron-dl currently only works in the main process, not in renderer processes. (I think it might be possible to fix that by using Electron's remote API in electron-dl.) Alternatively, maybe it's better to implement this without using any other package.
  2. Asynchronous language detection (spell-check-handler.js). In this case, electron-remote is used to run the CLD2 language detector in a separate process, asynchronously. This usage might be replaceable with Electron Web Workers. I think this would require that Node integration be enabled in Workers by any app that uses electron-spellchecker.
  3. Listening to 'context-menu' events from a window (context-menu-listener.js). I don't quite understand the motivation for using it here. Why is this necessary, and can this usage be removed?

What are people's thoughts on all this? I'm curious to learn more from the maintainers of this project. I'm working on a project that would like to use electron-spellchecker with Electron 5, so I've been trying to clear these hurdles. Thanks!

@kwonoj
Copy link
Contributor

kwonoj commented Jun 16, 2019

disclaimer: I am author of listed modules below.

To resolve issues listed around, I have wrote https://github.com/kwonoj/cld3-asm / https://github.com/kwonoj/electron-hunspell (and https://github.com/kwonoj/hunspell-dict-downloader as bonus module)

This separated module resolves each feature of spellchecker, notably

  • web assembly based (cld3 / hunspell), does not require any node integration features
  • support web workers
  • low-level api only focus on specific features (i.e do not expose context menu)
  • support latest electron (electron-hunspell provides interface to host spellchecker in worker thread)

for downloading dictionary, in the renderer process you really won't need main process - simple fetch would serve sufficiently.

@felixrieseberg
Copy link
Collaborator

While OJ's modules are probably the better solution for people in the long-term, I strongly agree that we should electron-remote now.

  1. Downloading Hunspell dictionaries (dictionary-sync.js). I think it might be better to use electron-dl for this. Unfortunately, electron-dl currently only works in the main process, not in renderer processes. (I think it might be possible to fix that by using Electron's remote API in electron-dl.) Alternatively, maybe it's better to implement this without using any other package.

I think we could just use fetch or Electron.net here - after all, we're just downloading stuff.

  1. Asynchronous language detection (spell-check-handler.js). In this case, electron-remote is used to run the CLD2 language detector in a separate process, asynchronously. This usage might be replaceable with Electron Web Workers. I think this would require that Node integration be enabled in Workers by any app that uses electron-spellchecker.

That's the biggie and the one place where doing the work in another process actually made sense - the other two were basically just using that process because "Hey, we have another process already anyway". If you'd like to stick with cld2, Node integration will indeed be a must. I wonder if we could just plug in OJ's module though.

@kwonoj This is possibly a silly question, but would replacing node-cld with cld3-asm require basically a complete rewrite of this module?

  1. Listening to 'context-menu' events from a window (context-menu-listener.js). I don't quite understand the motivation for using it here. Why is this necessary, and can this usage be removed?

Can totally be removed.

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

No branches or pull requests

3 participants