-
Notifications
You must be signed in to change notification settings - Fork 391
Update uses of Electron API calls that require Promises in Electron 7+ (callbacks are deprecated) #2624
Comments
API calls that may need updating:
|
Also see #2605. In general, passing tests isn't enough because the tests may be (now incorrectly) mocking the Electron interface. |
I feel like these tests mock maybe too much of the Electron API. Because I would have liked to see these tests fail earlier if the API usage is wrong. Better to do real operations and clean up after them, I think? (I'm also biased, because I'm not great at rewriting the tests. I find rewriting the tests more involved if you have to update all the mocks to be similar to real Electron after a major version Electron upgrade.) |
I agree in general, but don't know why they were written that way in the first place. Maybe the API can't be programmatically emulated, or caused flaky tests. @smashwilson might be able to comment on this. |
We have to mock I agree that I'd have liked to get earlier notice that these functions had changed signatures. I think the best way to do that would be using something like TypeScript types, though. |
I think I found all of them. Updated my earlier comment: #2624 (comment) |
I have a method to run this repo's tests against the "Electron 9 Atom" build artifacts from the Electron 9 PR. master...DeeDeeG:test-with-Atom-Electron-9 Feel free to open PRs at my fork against that branch to run the tests. Or @smashwilson if you would like to host a copy of that branch here it might make testing fixes easier. (Although I could optionally clean up the commit history of that branch quite a bit first.) |
i made this #2626 |
Do we have confirmation that we caught them all in an Electron 9 build so we can close this issue? 😄 |
i don't. I made a manual search and seems like that. But i do not feel like give it for granted. |
I also manually searched the I am confident we got all of those. So I do think this issue can be closed. (I was hoping this would lead to the |
Yes test continue to fail on windows ( inconsistently ) and on MacOs are a bummer. |
For what it's worth: In order to get some direction to pursue here, I am planning on getting my Electron 7 branch (with minimal diff from Atom Update: This repo's tests appear to pass with Electron 7: https://github.com/DeeDeeG/github/actions/runs/582856544 Update 2: Also passing in Atom's CI with Electron 7: https://dev.azure.com/DeeDeeG/b/_build/results?buildId=1103&view=logs&j=6de74252-975b-522d-7a3d-227ca79ca6c1&t=a70a2b1c-2767-57d7-6509-edd8e9181730&l=58 Now to find out if it's because of the "Electron 7" part or the "minimal diff from |
This documentation page seems to be better-maintained (than the Update: I went through all of the changes for Electron 7 (https://www.electronjs.org/docs/breaking-changes#planned-breaking-api-changes-70) and |
Some of these test failures begin with Electron 8, apparently: https://github.com/DeeDeeG/github/actions/runs/594334321 |
maybe we can do the same process with the electron 8 breaking changes IF the changes are already supported by electron 6. Otherwise the upgrade to electron 9 is not possible and we need to move from 6 to 7 first. |
This can definitely be closed thanks to #2631. Thanks @sadick254! 🎉 |
Summary
There are some Electron APIs that changed to only return Promises (and not run callbacks passed to them), mostly as of Electron 7+. Updating to be compatible with those would be great for updating Atom's Electron version.
See: https://www.electronjs.org/docs/api/modernization/promisification
Motivation
There is a Work In Progress pull request to update Atom to Electron 9, and I think updating these API calls may be necessary for the
github
package to continue to work with Electron 9. (atom/atom#21777)Describe alternatives you've considered
N/A
Additional context
Most (I think all) of these API calls generally support Promises starting with Electron 6. (Some of them support Promises as of even earlier, in Electron 5.) So implementing these changes should still be compatible with all the current and in-development versions of Atom.
github
's tests are failing over at Atom core's Electron 9 PR: atom/atom#21777 (comment)Error messages for the github package tests (click to expand):
The text was updated successfully, but these errors were encountered: