Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Revert to Electron 2 until we can overcome crashes on Electron 3 #18908

Merged
merged 4 commits into from
Feb 26, 2019

Conversation

jasonrudolph
Copy link
Contributor

@jasonrudolph jasonrudolph commented Feb 25, 2019

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 of script/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 to text-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

Copy link
Contributor

@smashwilson smashwilson left a 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-*
Copy link
Contributor

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.

Copy link
Contributor Author

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.

spec/decoration-manager-spec.coffee Show resolved Hide resolved
@@ -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'))
Copy link
Contributor

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?

Copy link
Contributor

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?

@jasonrudolph
Copy link
Contributor Author

  • Ensure all tests are passing

screen shot 2019-02-26 at 1 18 55 pm

The statuses above show that the Atom Pull Requests build for this branch is failing, but it's actually passing: https://github.visualstudio.com/Atom/_build/results?buildId=34110

2_26_19__1_20_pm

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. 😇

@jasonrudolph jasonrudolph merged commit 2712b1f into master Feb 26, 2019
@jasonrudolph jasonrudolph deleted the electron-2.0.16-revert branch February 26, 2019 18:25
jasonrudolph added a commit that referenced this pull request Feb 26, 2019
…hes on Electron 3"

This reverts commit 2712b1f, reversing
changes made to 042fb45.
@jasonrudolph jasonrudolph mentioned this pull request Feb 26, 2019
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants