electron-userland / electron-spellchecker

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

Remove electron-remote to fix support on Electron 5+ #170

Open bengotow opened 4 years ago

bengotow commented 4 years ago

Hey folks, per the discussion in #152, this PR:

      remote.getCurrentWebContents().addEventListener('context-menu', async (event, info) => {
        await contextMenuBuilder.showPopupMenu(info);
      });

Hope y'all are all doing well in these wild times!

implausible commented 4 years ago

I've updated cld to actually be async with a promise API in this PR https://github.com/dachev/node-cld/pull/62. Hopefully when that merges we'll legit not have to even worry about the difference between cld being async or not with respect to electron-spellchecker. Perhaps when this lands, you might be amenable to bringing that change into this PR? @bengotow

implausible commented 4 years ago

Replaces the use of electron-remote to download the dictionaries with a basic fetch.

So I realize that electron-remote/remote-ajax leveraged fetch in the past, and this is a very 1-to-1 change for this minus the obvious difference, but it might actually be wiser to leverage XHR for this operation right now. It's kind of hard to track as there's not a lot of info out there on it, but window.fetch does not go through the same network layer that XHR goes through. The particular layer it does go through is lacking support for proxied connections. This is a bit of an old mention on the issue: https://discuss.atom.io/t/window-fetch-not-following-system-network-configuration/39139 but I am fairly confident this is still relevant today as the last time we checked this issue internally was ~3 months ago.