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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bengotow
Copy link
Contributor

@bengotow bengotow commented Apr 20, 2020

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

  • Replaces the use of electron-remote to download the dictionaries with a basic fetch. I manually checked that the files this produces on disk have the correct checksums because the buffer conversion you have to do from fetch => fs is a little whacky.

  • Removes the use of electron-remote task pooling around cld language detection. I actually think that this has been unnecessary for a long time - the spellchecker truncates the provided text to 512 characters, so the run time of CLD has a fairly low upper bound. I use cld synchronously in another place in Mailspring and with a character limit that low it's never been an issue. This code is only run when the user of the module calls provideHintText, so maybe just don't call it constantly - you shouldn't have been doing that before anyway. ;-)

  • Removes the ContextMenuListener from this module entirely. I think it was included because doing it "right" was hard, but I don't think that's the case anymore? I believe you can just replace your usage with this - I believe Electron has fixed the "listeners getting detached" issue that was originally being worked around via electron-remote.

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

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

# Conflicts:
#	src/context-menu-listener.js
#	src/dictionary-sync.js
#	src/spell-check-handler.js
# Conflicts:
#	example/index.html
#	package-lock.json
@implausible
Copy link

I've updated cld to actually be async with a promise API in this PR dachev/node-cld#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
Copy link

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.

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

Successfully merging this pull request may close these issues.

2 participants