electron-userland / electron-spellchecker

Implement spellchecking, correctly
MIT License
239 stars 83 forks source link

Replace dependency on deprecated electron-remote package #152

Open mlalkaka opened 5 years ago

mlalkaka commented 5 years ago

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 commented 5 years ago

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

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

felixrieseberg commented 5 years ago

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.