-
Notifications
You must be signed in to change notification settings - Fork 17.4k
Revert to Electron 2 until we can overcome crashes on Electron 3 #18908
Conversation
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.
👍 Nothing in here that should break the changes we've made since then, at a glance anyway. I guess we'll find out pretty quickly what we missed 😅
@@ -1,6 +1,7 @@ | |||
trigger: | |||
- master | |||
- 1.* # VSTS only supports wildcards at the end | |||
- electron-* |
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.
👍 This will be useful in the future. We'll have to make sure we don't revert it away when we revert these reversions.
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.
Good point. I've opened #18916 to revert these reversions 🥴💫😵, but this change is still intact in that pull request.
@@ -2086,7 +2086,7 @@ describe('Workspace', () => { | |||
}) | |||
|
|||
runs(() => { | |||
fs.renameSync(path.join(projectPath, 'git.git'), path.join(projectPath, '.git')) | |||
fs.rename(path.join(projectPath, 'git.git'), path.join(projectPath, '.git')) |
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.
This change looks odd to me. Was fs.rename
always synchronous in the old version of fs-extra?
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.
Prior to Node 10 fs.rename
accepted a callback parameter and didn't complain if you didn't provide a callback. In Node 10 it seems that the callback is now mandatory if you use the non-Sync variant. I suppose the function worked as synchronous before if no callback was specified?
The statuses above show that the I'm not sure why visualstudio.com isn't reporting the passing build to GitHub, but solving that is out of scope for this pull request. 😇 |
Description of the Change
#18603 and #18815 upgraded Atom from Electron 2 to Electron 3. (Specifically, we went from 2.0.16 to 3.1.13.) Those PRs satisfied the initial "smoke tests" enumerated in #18603 (comment), but we've unfortunately been experiencing frequent crashes when using Atom day-to-day on Electron 3 (e.g., #18790, #18837, #18841) and when running on CI.
Ideally, we want to resolve those issues and move forward with Electron 3, but it will take some time to resolve those issues. In the meantime, we need Atom's master branch to provide a more usable version of Atom. 😇 With that in mind, this pull request temporarily reverts #18603 and #18815, thus restoring Atom to its previous Electron version (2.0.16). By giving us a more stable master branch, this pull request will unblock our ability to ship Atom 1.36-beta0.
Alternate Designs
Instead of reverting all of the changes in #18603 and #18815, I first attempted to only change the Electron versions in
package.json
. (See 042fb45...f5349eb.) To get Atom to build locally after that change, I also had to revert to the previous version ofscript/lib/generate-startup-snapshot.js
, as explained in the commit message in 2fd9356. Those changes were sufficient to build Atom, but a large number of tests fail related totext-editor-component.js
.Possible Drawbacks
We'll have to undo these reversions when we're ready to move forward with the Electron 3 upgrade.
Verification Process