From 69e631d5096b84a2b910713ec9bbf2ddf6a215f9 Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Tue, 5 Sep 2017 16:12:12 -0700 Subject: [PATCH 01/11] Allow Promises to be returned by a package deactivate method --- benchmarks/benchmark-runner.js | 2 +- spec/spec-helper.coffee | 12 ++++++++---- src/atom-environment.coffee | 6 +++++- src/package-manager.js | 16 ++++++++-------- src/package.coffee | 13 ++++++++----- 5 files changed, 30 insertions(+), 19 deletions(-) diff --git a/benchmarks/benchmark-runner.js b/benchmarks/benchmark-runner.js index 30b23ffbf..56a37cfd4 100644 --- a/benchmarks/benchmark-runner.js +++ b/benchmarks/benchmark-runner.js @@ -65,7 +65,7 @@ export default async function ({test, benchmarkPaths}) { console.log(textualOutput) } - global.atom.reset() + await global.atom.reset() } } diff --git a/spec/spec-helper.coffee b/spec/spec-helper.coffee index eec8ce5fb..c20bfc827 100644 --- a/spec/spec-helper.coffee +++ b/spec/spec-helper.coffee @@ -108,10 +108,14 @@ beforeEach -> afterEach -> ensureNoDeprecatedFunctionCalls() ensureNoDeprecatedStylesheets() - atom.reset() - document.getElementById('jasmine-content').innerHTML = '' unless window.debugContent - warnIfLeakingPathSubscriptions() - waits(0) # yield to ui thread to make screen update more frequently + + waitsForPromise -> + atom.reset() + + runs -> + document.getElementById('jasmine-content').innerHTML = '' unless window.debugContent + warnIfLeakingPathSubscriptions() + waits(0) # yield to ui thread to make screen update more frequently warnIfLeakingPathSubscriptions = -> watchedPaths = pathwatcher.getWatchedPaths() diff --git a/src/atom-environment.coffee b/src/atom-environment.coffee index b37acddd1..7e425edcd 100644 --- a/src/atom-environment.coffee +++ b/src/atom-environment.coffee @@ -702,6 +702,11 @@ class AtomEnvironment extends Model windowCloseRequested: true, projectHasPaths: @project.getPaths().length > 0 }) + .then (closing) => + if closing + @packages.deactivatePackages().then -> closing + else + closing @listenForUpdates() @@ -758,7 +763,6 @@ class AtomEnvironment extends Model return if not @project @storeWindowBackground() - @packages.deactivatePackages() @saveBlobStoreSync() @unloaded = true diff --git a/src/package-manager.js b/src/package-manager.js index 73855ae37..7718c8618 100644 --- a/src/package-manager.js +++ b/src/package-manager.js @@ -77,9 +77,9 @@ module.exports = class PackageManager { this.themeManager = themeManager } - reset () { + async reset () { this.serviceHub.clear() - this.deactivatePackages() + await this.deactivatePackages() this.loadedPackages = {} this.preloadedPackages = {} this.packageStates = {} @@ -744,21 +744,21 @@ module.exports = class PackageManager { } // Deactivate all packages - deactivatePackages () { - this.config.transact(() => { - this.getLoadedPackages().forEach(pack => this.deactivatePackage(pack.name, true)) - }) + async deactivatePackages () { + await this.config.transactAsync(() => + Promise.all(this.getLoadedPackages().map(pack => this.deactivatePackage(pack.name, true))) + ) this.unobserveDisabledPackages() this.unobservePackagesWithKeymapsDisabled() } // Deactivate the package with the given name - deactivatePackage (name, suppressSerialization) { + async deactivatePackage (name, suppressSerialization) { const pack = this.getLoadedPackage(name) if (!suppressSerialization && this.isPackageActive(pack.name)) { this.serializePackage(pack) } - pack.deactivate() + await pack.deactivate() delete this.activePackages[pack.name] delete this.activatingPackages[pack.name] this.emitter.emit('did-deactivate-package', pack) diff --git a/src/package.coffee b/src/package.coffee index 039ccf9d3..556847bc9 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -506,15 +506,18 @@ class Package @configSchemaRegisteredOnActivate = false @deactivateResources() @deactivateKeymaps() + result = Promise.resolve() if @mainActivated try - @mainModule?.deactivate?() - @mainModule?.deactivateConfig?() - @mainActivated = false - @mainInitialized = false + result = Promise.resolve(@mainModule?.deactivate?()).then => + @mainModule?.deactivateConfig?() + @mainActivated = false + @mainInitialized = false catch e console.error "Error deactivating package '#{@name}'", e.stack - @emitter.emit 'did-deactivate' + result = result.then => + @emitter.emit 'did-deactivate' + result deactivateResources: -> grammar.deactivate() for grammar in @grammars From 9898f6b36c706c6d4634abe4e66108875acd481e Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Wed, 6 Sep 2017 11:10:39 -0700 Subject: [PATCH 02/11] Ensure atom.global.reset returns a promise --- src/atom-environment.coffee | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/atom-environment.coffee b/src/atom-environment.coffee index 7e425edcd..fd9af9342 100644 --- a/src/atom-environment.coffee +++ b/src/atom-environment.coffee @@ -328,20 +328,14 @@ class AtomEnvironment extends Model @contextMenu.clear() - @packages.reset() - - @workspace.reset(@packages) - @registerDefaultOpeners() - - @project.reset(@packages) - - @workspace.subscribeToEvents() - - @grammars.clear() - - @textEditors.clear() - - @views.clear() + @packages.reset().then -> + @workspace.reset(@packages) + @registerDefaultOpeners() + @project.reset(@packages) + @workspace.subscribeToEvents() + @grammars.clear() + @textEditors.clear() + @views.clear() destroy: -> return if not @project From 0a2ff530ff7572e24650c246cd7b2ae051b58c78 Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Wed, 6 Sep 2017 14:15:26 -0700 Subject: [PATCH 03/11] Get more tests working after async --- spec/grammars-spec.coffee | 10 ++-- spec/language-mode-spec.coffee | 48 ++++++++++++------- spec/package-manager-spec.coffee | 79 ++++++++++++++++++++++++++------ spec/theme-manager-spec.coffee | 8 ++-- src/atom-environment.coffee | 2 +- src/theme-manager.coffee | 3 +- 6 files changed, 111 insertions(+), 39 deletions(-) diff --git a/spec/grammars-spec.coffee b/spec/grammars-spec.coffee index 7d4754397..7b70797ba 100644 --- a/spec/grammars-spec.coffee +++ b/spec/grammars-spec.coffee @@ -22,10 +22,12 @@ describe "the `grammars` global", -> atom.packages.activatePackage('language-git') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() - try - temp.cleanupSync() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() + try + temp.cleanupSync() describe ".selectGrammar(filePath)", -> it "always returns a grammar", -> diff --git a/spec/language-mode-spec.coffee b/spec/language-mode-spec.coffee index 4025472af..aca96d2cc 100644 --- a/spec/language-mode-spec.coffee +++ b/spec/language-mode-spec.coffee @@ -15,8 +15,10 @@ describe "LanguageMode", -> atom.packages.activatePackage('language-javascript') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() describe ".minIndentLevelForRowRange(startRow, endRow)", -> it "returns the minimum indent level for the given row range", -> @@ -175,8 +177,10 @@ describe "LanguageMode", -> atom.packages.activatePackage('language-coffee-script') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() describe ".toggleLineCommentsForBufferRows(start, end)", -> it "comments/uncomments lines in the given range", -> @@ -222,8 +226,10 @@ describe "LanguageMode", -> atom.packages.activatePackage('language-css') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() describe ".toggleLineCommentsForBufferRows(start, end)", -> it "comments/uncomments lines in the given range", -> @@ -274,8 +280,10 @@ describe "LanguageMode", -> atom.packages.activatePackage('language-css') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() describe "when commenting lines", -> it "only uses the `commentEnd` pattern if it comes from the same grammar as the `commentStart`", -> @@ -294,8 +302,10 @@ describe "LanguageMode", -> atom.packages.activatePackage('language-xml') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() describe "when uncommenting lines", -> it "removes the leading whitespace from the comment end pattern match", -> @@ -313,8 +323,10 @@ describe "LanguageMode", -> atom.packages.activatePackage('language-javascript') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() it "maintains cursor buffer position when a folding/unfolding", -> editor.setCursorBufferPosition([5, 5]) @@ -403,8 +415,10 @@ describe "LanguageMode", -> atom.packages.activatePackage('language-javascript') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() describe ".unfoldAll()", -> it "unfolds every folded line", -> @@ -481,8 +495,10 @@ describe "LanguageMode", -> atom.packages.activatePackage('language-css') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() describe "suggestedIndentForBufferRow", -> it "does not return negative values (regression)", -> diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index 88efff4ae..294a2bfac 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -56,8 +56,10 @@ describe "PackageManager", -> spyOn(atom.packages, 'loadAvailablePackage') afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() it "sets hasLoadedInitialPackages", -> expect(atom.packages.hasLoadedInitialPackages()).toBe false @@ -172,8 +174,10 @@ describe "PackageManager", -> model2 = {worksWithViewProvider2: true} afterEach -> - atom.packages.deactivatePackage('package-with-view-providers') - atom.packages.unloadPackage('package-with-view-providers') + waitsForPromise -> + atom.packages.deactivatePackage('package-with-view-providers') + runs -> + atom.packages.unloadPackage('package-with-view-providers') it "does not load the view providers immediately", -> pack = atom.packages.loadPackage("package-with-view-providers") @@ -641,7 +645,11 @@ describe "PackageManager", -> runs -> expect(mainModule.activate.callCount).toBe 1 + + waitsForPromise -> atom.packages.deactivatePackage('package-with-activation-hooks') + + runs -> promise = atom.packages.activatePackage('package-with-activation-hooks') atom.packages.triggerActivationHook('language-fictitious:grammar-used') atom.packages.triggerDeferredActivationHooks() @@ -704,7 +712,9 @@ describe "PackageManager", -> expect(pack.mainModule.someNumber).not.toBe 77 pack.mainModule.someNumber = 77 atom.packages.serializePackage("package-with-serialization") + waitsForPromise -> atom.packages.deactivatePackage("package-with-serialization") + runs -> spyOn(pack.mainModule, 'activate').andCallThrough() waitsForPromise -> atom.packages.activatePackage("package-with-serialization") @@ -872,6 +882,7 @@ describe "PackageManager", -> expect(events.length).toBe(1) expect(events[0].type).toBe("user-command") + waitsForPromise -> atom.packages.deactivatePackage("package-with-keymaps") waitsForPromise -> @@ -1041,12 +1052,15 @@ describe "PackageManager", -> consumerModule.consumeFirstServiceV4.reset() consumerModule.consumeSecondService.reset() + waitsForPromise -> atom.packages.deactivatePackage("package-with-provided-services") + runs -> expect(firstServiceV3Disposed).toBe true expect(firstServiceV4Disposed).toBe true expect(secondServiceDisposed).toBe true + waitsForPromise -> atom.packages.deactivatePackage("package-with-consumed-services") waitsForPromise -> @@ -1112,8 +1126,11 @@ describe "PackageManager", -> runs -> spyOn(pack1.mainModule, 'deactivate') spyOn(pack2.mainModule, 'serialize') + + waitsForPromise -> atom.packages.deactivatePackages() + runs -> expect(pack1.mainModule.deactivate).toHaveBeenCalled() expect(pack2.mainModule.serialize).not.toHaveBeenCalled() @@ -1131,7 +1148,10 @@ describe "PackageManager", -> expect(atom.packages.isPackageActive("package-with-deactivate")).toBeTruthy() spyOn(pack.mainModule, 'deactivate').andCallThrough() + waitsForPromise -> atom.packages.deactivatePackage("package-with-deactivate") + + runs -> expect(pack.mainModule.deactivate).toHaveBeenCalled() expect(atom.packages.isPackageActive("package-with-module")).toBeFalsy() @@ -1145,26 +1165,38 @@ describe "PackageManager", -> expect(atom.packages.isPackageActive("package-that-throws-on-activate")).toBeTruthy() spyOn(badPack.mainModule, 'deactivate').andCallThrough() + waitsForPromise -> atom.packages.deactivatePackage("package-that-throws-on-activate") + + runs -> expect(badPack.mainModule.deactivate).not.toHaveBeenCalled() expect(atom.packages.isPackageActive("package-that-throws-on-activate")).toBeFalsy() it "absorbs exceptions that are thrown by the package module's deactivate method", -> spyOn(console, 'error') + thrownError = null waitsForPromise -> atom.packages.activatePackage("package-that-throws-on-deactivate") + waitsForPromise -> + try + atom.packages.deactivatePackage("package-that-throws-on-deactivate") + catch error + thrownError = error + runs -> - expect(-> atom.packages.deactivatePackage("package-that-throws-on-deactivate")).not.toThrow() + expect(thrownError).toBeNull() expect(console.error).toHaveBeenCalled() it "removes the package's grammars", -> waitsForPromise -> atom.packages.activatePackage('package-with-grammars') - runs -> + waitsForPromise -> atom.packages.deactivatePackage('package-with-grammars') + + runs -> expect(atom.grammars.selectGrammar('a.alot').name).toBe 'Null Grammar' expect(atom.grammars.selectGrammar('a.alittle').name).toBe 'Null Grammar' @@ -1172,8 +1204,10 @@ describe "PackageManager", -> waitsForPromise -> atom.packages.activatePackage('package-with-keymaps') - runs -> + waitsForPromise -> atom.packages.deactivatePackage('package-with-keymaps') + + runs -> expect(atom.keymaps.findKeyBindings(keystrokes: 'ctrl-z', target: createTestElement('test-1'))).toHaveLength 0 expect(atom.keymaps.findKeyBindings(keystrokes: 'ctrl-z', target: createTestElement('test-2'))).toHaveLength 0 @@ -1181,8 +1215,10 @@ describe "PackageManager", -> waitsForPromise -> atom.packages.activatePackage('package-with-styles') - runs -> + waitsForPromise -> atom.packages.deactivatePackage('package-with-styles') + + runs -> one = require.resolve("./fixtures/packages/package-with-style-sheets-manifest/styles/1.css") two = require.resolve("./fixtures/packages/package-with-style-sheets-manifest/styles/2.less") three = require.resolve("./fixtures/packages/package-with-style-sheets-manifest/styles/3.css") @@ -1196,17 +1232,26 @@ describe "PackageManager", -> runs -> expect(atom.config.get 'editor.increaseIndentPattern', scope: ['.source.omg']).toBe '^a' + + waitsForPromise -> atom.packages.deactivatePackage("package-with-settings") + + runs -> expect(atom.config.get 'editor.increaseIndentPattern', scope: ['.source.omg']).toBeUndefined() it "invokes ::onDidDeactivatePackage listeners with the deactivated package", -> + deactivatedPackage = null + waitsForPromise -> atom.packages.activatePackage("package-with-main") runs -> - deactivatedPackage = null atom.packages.onDidDeactivatePackage (pack) -> deactivatedPackage = pack + + waitsForPromise -> atom.packages.deactivatePackage("package-with-main") + + runs -> expect(deactivatedPackage.name).toBe "package-with-main" describe "::activate()", -> @@ -1220,10 +1265,11 @@ describe "PackageManager", -> expect(loadedPackages.length).toBeGreaterThan 0 afterEach -> - atom.packages.deactivatePackages() - atom.packages.unloadPackages() - - jasmine.restoreDeprecationsSnapshot() + waitsForPromise -> + atom.packages.deactivatePackages() + runs -> + atom.packages.unloadPackages() + jasmine.restoreDeprecationsSnapshot() it "sets hasActivatedInitialPackages", -> spyOn(atom.styles, 'getUserStyleSheetPath').andReturn(null) @@ -1286,6 +1332,9 @@ describe "PackageManager", -> it "disables an enabled package", -> packageName = 'package-with-main' + pack = null + activatedPackages = null + waitsForPromise -> atom.packages.activatePackage(packageName) @@ -1295,7 +1344,11 @@ describe "PackageManager", -> pack = atom.packages.disablePackage(packageName) + waitsFor -> activatedPackages = atom.packages.getActivePackages() + activatedPackages.length is 0 + + runs -> expect(activatedPackages).not.toContain(pack) expect(atom.config.get('core.disabledPackages')).toContain packageName diff --git a/spec/theme-manager-spec.coffee b/spec/theme-manager-spec.coffee index 5d2912f5b..86237b71d 100644 --- a/spec/theme-manager-spec.coffee +++ b/spec/theme-manager-spec.coffee @@ -8,9 +8,11 @@ describe "atom.themes", -> spyOn(console, 'warn') afterEach -> - atom.themes.deactivateThemes() - try - temp.cleanupSync() + waitsForPromise -> + atom.themes.deactivateThemes() + runs -> + try + temp.cleanupSync() describe "theme getters and setters", -> beforeEach -> diff --git a/src/atom-environment.coffee b/src/atom-environment.coffee index fd9af9342..bd631435e 100644 --- a/src/atom-environment.coffee +++ b/src/atom-environment.coffee @@ -328,7 +328,7 @@ class AtomEnvironment extends Model @contextMenu.clear() - @packages.reset().then -> + @packages.reset().then => @workspace.reset(@packages) @registerDefaultOpeners() @project.reset(@packages) diff --git a/src/theme-manager.coffee b/src/theme-manager.coffee index 0f019dbf0..b6e36ba5b 100644 --- a/src/theme-manager.coffee +++ b/src/theme-manager.coffee @@ -287,8 +287,7 @@ class ThemeManager deactivateThemes: -> @removeActiveThemeClasses() @unwatchUserStylesheet() - @packageManager.deactivatePackage(pack.name) for pack in @getActiveThemes() - null + Promise.all(@packageManager.deactivatePackage(pack.name) for pack in @getActiveThemes()) isInitialLoadComplete: -> @initialLoadComplete From af66c5efaabf3d52e4ae7aa1d92d60e6d5c3810b Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Wed, 6 Sep 2017 14:52:08 -0700 Subject: [PATCH 04/11] Fix two more tests --- spec/package-spec.coffee | 9 ++++++--- spec/text-editor-registry-spec.js | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/spec/package-spec.coffee b/spec/package-spec.coffee index 5018d2eb4..0a6ee01f9 100644 --- a/spec/package-spec.coffee +++ b/spec/package-spec.coffee @@ -138,7 +138,8 @@ describe "Package", -> jasmine.attachToDOM(editorElement) afterEach -> - theme.deactivate() if theme? + waitsForPromise -> + theme.deactivate() if theme? describe "when the theme contains a single style file", -> it "loads and applies css", -> @@ -200,8 +201,10 @@ describe "Package", -> it "deactivated event fires on .deactivate()", -> theme.onDidDeactivate spy = jasmine.createSpy() - theme.deactivate() - expect(spy).toHaveBeenCalled() + waitsForPromise -> + theme.deactivate() + runs -> + expect(spy).toHaveBeenCalled() describe ".loadMetadata()", -> [packagePath, metadata] = [] diff --git a/spec/text-editor-registry-spec.js b/spec/text-editor-registry-spec.js index 79d575a5f..2479bff9b 100644 --- a/spec/text-editor-registry-spec.js +++ b/spec/text-editor-registry-spec.js @@ -685,7 +685,7 @@ describe('TextEditorRegistry', function () { registry.setGrammarOverride(editor, 'source.c') registry.setGrammarOverride(editor2, 'source.js') - atom.packages.deactivatePackage('language-javascript') + await atom.packages.deactivatePackage('language-javascript') const editorCopy = TextEditor.deserialize(editor.serialize(), atom) const editor2Copy = TextEditor.deserialize(editor2.serialize(), atom) From 468f4a47a92292167cf4f5982af691095650c3de Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Wed, 6 Sep 2017 15:28:58 -0700 Subject: [PATCH 05/11] Fix another two tests --- spec/package-manager-spec.coffee | 3 ++- src/theme-manager.coffee | 36 +++++++++++++++----------------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index 294a2bfac..d0051a452 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -1375,7 +1375,8 @@ describe "PackageManager", -> atom.themes.activateThemes() afterEach -> - atom.themes.deactivateThemes() + waitsForPromise -> + atom.themes.deactivateThemes() it "enables and disables a theme", -> packageName = 'theme-with-package-file' diff --git a/src/theme-manager.coffee b/src/theme-manager.coffee index b6e36ba5b..742b86ddd 100644 --- a/src/theme-manager.coffee +++ b/src/theme-manager.coffee @@ -262,27 +262,25 @@ class ThemeManager new Promise (resolve) => # @config.observe runs the callback once, then on subsequent changes. @config.observe 'core.themes', => - @deactivateThemes() + @deactivateThemes().then => + @warnForNonExistentThemes() + @refreshLessCache() # Update cache for packages in core.themes config - @warnForNonExistentThemes() + promises = [] + for themeName in @getEnabledThemeNames() + if @packageManager.resolvePackagePath(themeName) + promises.push(@packageManager.activatePackage(themeName)) + else + console.warn("Failed to activate theme '#{themeName}' because it isn't installed.") - @refreshLessCache() # Update cache for packages in core.themes config - - promises = [] - for themeName in @getEnabledThemeNames() - if @packageManager.resolvePackagePath(themeName) - promises.push(@packageManager.activatePackage(themeName)) - else - console.warn("Failed to activate theme '#{themeName}' because it isn't installed.") - - Promise.all(promises).then => - @addActiveThemeClasses() - @refreshLessCache() # Update cache again now that @getActiveThemes() is populated - @loadUserStylesheet() - @reloadBaseStylesheets() - @initialLoadComplete = true - @emitter.emit 'did-change-active-themes' - resolve() + Promise.all(promises).then => + @addActiveThemeClasses() + @refreshLessCache() # Update cache again now that @getActiveThemes() is populated + @loadUserStylesheet() + @reloadBaseStylesheets() + @initialLoadComplete = true + @emitter.emit 'did-change-active-themes' + resolve() deactivateThemes: -> @removeActiveThemeClasses() From 79fbef8e2463ecf61ec26d3a4c0e7eea249dcba9 Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Wed, 6 Sep 2017 16:06:17 -0700 Subject: [PATCH 06/11] :shirt: --- spec/package-manager-spec.coffee | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index d0051a452..9b7d46340 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -1181,7 +1181,7 @@ describe "PackageManager", -> waitsForPromise -> try - atom.packages.deactivatePackage("package-that-throws-on-deactivate") + atom.packages.deactivatePackage("package-that-throws-on-deactivate") catch error thrownError = error @@ -1375,8 +1375,8 @@ describe "PackageManager", -> atom.themes.activateThemes() afterEach -> - waitsForPromise -> - atom.themes.deactivateThemes() + waitsForPromise -> + atom.themes.deactivateThemes() it "enables and disables a theme", -> packageName = 'theme-with-package-file' From 524d483610baaa074c47769ef5ddfff62b26ca73 Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Fri, 8 Sep 2017 11:20:23 -0700 Subject: [PATCH 07/11] Ensure non-async deactivate is run syncronously without await --- src/package-manager.js | 7 ++++++- src/package.coffee | 34 +++++++++++++++++++++++----------- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/package-manager.js b/src/package-manager.js index 7718c8618..0f4484a5b 100644 --- a/src/package-manager.js +++ b/src/package-manager.js @@ -758,7 +758,12 @@ module.exports = class PackageManager { if (!suppressSerialization && this.isPackageActive(pack.name)) { this.serializePackage(pack) } - await pack.deactivate() + + const deactivationResult = pack.deactivate() + if (deactivationResult && typeof deactivationResult.then === 'function') { + await deactivationResult; + } + delete this.activePackages[pack.name] delete this.activatingPackages[pack.name] this.emitter.emit('did-deactivate-package', pack) diff --git a/src/package.coffee b/src/package.coffee index 556847bc9..fdd89bc74 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -506,18 +506,30 @@ class Package @configSchemaRegisteredOnActivate = false @deactivateResources() @deactivateKeymaps() - result = Promise.resolve() - if @mainActivated - try - result = Promise.resolve(@mainModule?.deactivate?()).then => - @mainModule?.deactivateConfig?() - @mainActivated = false - @mainInitialized = false - catch e - console.error "Error deactivating package '#{@name}'", e.stack - result = result.then => + + unless @mainActivated @emitter.emit 'did-deactivate' - result + return + + try + deactivationResult = @mainModule?.deactivate?() + catch e + console.error "Error deactivating package '#{@name}'", e.stack + + # We support then-able async promises as well as sync ones from deactivate + if deactivationResult?.then is 'function' + deactivationResult.then => @afterDeactivation() + else + @afterDeactivation() + + afterDeactivation: -> + try + @mainModule?.deactivateConfig?() + catch e + console.error "Error deactivating package '#{@name}'", e.stack + @mainActivated = false + @mainInitialized = false + @emitter.emit 'did-deactivate' deactivateResources: -> grammar.deactivate() for grammar in @grammars From 199bab89934fec7198cce0ae39fdaa1ae7087803 Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Fri, 8 Sep 2017 13:19:36 -0700 Subject: [PATCH 08/11] Fix a few more tests --- spec/package-spec.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/package-spec.coffee b/spec/package-spec.coffee index 0a6ee01f9..e7bfd0249 100644 --- a/spec/package-spec.coffee +++ b/spec/package-spec.coffee @@ -139,7 +139,7 @@ describe "Package", -> afterEach -> waitsForPromise -> - theme.deactivate() if theme? + Promise.resolve(theme.deactivate()) if theme? describe "when the theme contains a single style file", -> it "loads and applies css", -> @@ -202,7 +202,7 @@ describe "Package", -> it "deactivated event fires on .deactivate()", -> theme.onDidDeactivate spy = jasmine.createSpy() waitsForPromise -> - theme.deactivate() + Promise.resolve(theme.deactivate()) runs -> expect(spy).toHaveBeenCalled() From dab150ad6caeaed269acd13696dfb71360d61fc6 Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Fri, 8 Sep 2017 14:10:35 -0700 Subject: [PATCH 09/11] :tshirt: --- src/package-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/package-manager.js b/src/package-manager.js index 0f4484a5b..da5c1261c 100644 --- a/src/package-manager.js +++ b/src/package-manager.js @@ -761,7 +761,7 @@ module.exports = class PackageManager { const deactivationResult = pack.deactivate() if (deactivationResult && typeof deactivationResult.then === 'function') { - await deactivationResult; + await deactivationResult } delete this.activePackages[pack.name] From a21480f441025bd1819768058c76f65e59b42a3f Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Fri, 8 Sep 2017 16:10:09 -0700 Subject: [PATCH 10/11] Better error handling on passing invalid packs --- src/package-manager.js | 4 ++++ src/theme-manager.coffee | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/package-manager.js b/src/package-manager.js index da5c1261c..b52e29cad 100644 --- a/src/package-manager.js +++ b/src/package-manager.js @@ -755,6 +755,10 @@ module.exports = class PackageManager { // Deactivate the package with the given name async deactivatePackage (name, suppressSerialization) { const pack = this.getLoadedPackage(name) + if (pack == null) { + return + } + if (!suppressSerialization && this.isPackageActive(pack.name)) { this.serializePackage(pack) } diff --git a/src/theme-manager.coffee b/src/theme-manager.coffee index 742b86ddd..062e725c7 100644 --- a/src/theme-manager.coffee +++ b/src/theme-manager.coffee @@ -285,7 +285,8 @@ class ThemeManager deactivateThemes: -> @removeActiveThemeClasses() @unwatchUserStylesheet() - Promise.all(@packageManager.deactivatePackage(pack.name) for pack in @getActiveThemes()) + results = @getActiveThemes().map((pack) => @packageManager.deactivatePackage(pack.name)) + Promise.all(results.filter((r) => typeof r?.then is 'function')) isInitialLoadComplete: -> @initialLoadComplete From 69eb2d541dd5cb43e8126209026e5f555541075b Mon Sep 17 00:00:00 2001 From: Damien Guard Date: Sat, 9 Sep 2017 09:28:43 -0700 Subject: [PATCH 11/11] :t-shirt: --- src/theme-manager.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/theme-manager.coffee b/src/theme-manager.coffee index 062e725c7..d6a67ee61 100644 --- a/src/theme-manager.coffee +++ b/src/theme-manager.coffee @@ -286,7 +286,7 @@ class ThemeManager @removeActiveThemeClasses() @unwatchUserStylesheet() results = @getActiveThemes().map((pack) => @packageManager.deactivatePackage(pack.name)) - Promise.all(results.filter((r) => typeof r?.then is 'function')) + Promise.all(results.filter((r) -> typeof r?.then is 'function')) isInitialLoadComplete: -> @initialLoadComplete