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
fix: Update spellcheck API in smoketest #116
Conversation
You'll need to update the docs hash for the tests to pass https://github.com/electron/electron-typescript-definitions/blob/master/vendor/fetch-docs.jsL You can point it at your PR |
🤘 |
test-smoke/electron/test/renderer.ts
Outdated
webFrame.setSpellCheckProvider('en-US', { | ||
spellCheck (words, callback) { | ||
setTimeout(() => { | ||
let misspeltWords = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- nit: `misspelt' is valid but has 1/28th the Google hits as 'misspelled', and the spellchecker API uses the word 'misspelled', so let's follow the Principle of Least Surprise and use it here too
const
in JS means "we won't change the address this pointer points to" rather than "this variable's value is immutable", so this can & should beconst
rather thanlet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually using misspelt in the updated spellcheck API PR in electron. Changed that also.
test-smoke/electron/test/renderer.ts
Outdated
setTimeout(() => { | ||
let misspeltWords = [] | ||
for (let word of words) { | ||
if (require('spellchecker').isMisspelled(word)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require()
has a nontrivial cost, so it's better to move it outside of this loop into a localconst
Array.prototype.filter()
would be useful here
test-smoke/electron/test/renderer.ts
Outdated
misspeltWords.push(word) | ||
} | ||
} | ||
callback(misspeltWords) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more concise:
const spellchecker = require('spellchecker')
const misspelled = words.filter(x => spellchecker.isMisspelled(x))
callback(misspelled)
Thanks for the feedback @ckerr ! 🙇 |
spellCheck (words, callback) { | ||
setTimeout(() => { | ||
const spellchecker = require('spellchecker') | ||
const misspelled = words.filter(x => spellchecker.isMisspelled(x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know misspelt
is "a word", but I'm glad to see that it got refactored away. 😛
Updates the spellcheck API in smoke tests.
[Corresponding electron change https://github.com/electron/electron/pull/14032]