Merge pull request #4755 from atom/mb-fix-disabled-package-situation

Fix recurring errors when a non-existent package is disabled

spec/config-spec.coffee

@@ -493,13 +493,19 @@ describe "Config", ->
         expect(observeHandler).not.toHaveBeenCalled()
 
       it "fires the callback every time the observed value changes", ->
-        observeHandler.reset() # clear the initial call
         atom.config.set('foo.bar.baz', "value 2")
         expect(observeHandler).toHaveBeenCalledWith({newValue: 'value 2', oldValue: 'value 1'})
         observeHandler.reset()
 
-        atom.config.set('foo.bar.baz', "value 1")
+        observeHandler.andCallFake -> throw new Error("oops")
+        expect(-> atom.config.set('foo.bar.baz', "value 1")).toThrow("oops")
         expect(observeHandler).toHaveBeenCalledWith({newValue: 'value 1', oldValue: 'value 2'})
+        observeHandler.reset()
+
+        # Regression: exception in earlier handler shouldn't put observer
+        # into a bad state.
+        atom.config.set('something.else', "new value")
+        expect(observeHandler).not.toHaveBeenCalled()
 
     describe 'when a keyPath is not specified', ->
       beforeEach ->
@@ -567,7 +573,7 @@ describe "Config", ->
       atom.config.set("foo.bar.baz", "i'm back")
       expect(observeHandler).toHaveBeenCalledWith("i'm back")
 
-    it "does not fire the callback once the observe subscription is off'ed", ->
+    it "does not fire the callback once the subscription is disposed", ->
       observeHandler.reset() # clear the initial call
       observeSubscription.dispose()
       atom.config.set('foo.bar.baz', "value 2")

spec/package-manager-spec.coffee

@@ -3,19 +3,36 @@ Package = require '../src/package'
 
 describe "PackageManager", ->
   workspaceElement = null
+
   beforeEach ->
     workspaceElement = atom.views.getView(atom.workspace)
 
   describe "::loadPackage(name)", ->
-    it "continues if the package has an invalid package.json", ->
-      spyOn(console, 'warn')
+    beforeEach ->
       atom.config.set("core.disabledPackages", [])
-      expect(-> atom.packages.loadPackage("package-with-broken-package-json")).not.toThrow()
 
-    it "continues if the package has an invalid keymap", ->
+    it "returns the package", ->
+      pack = atom.packages.loadPackage("package-with-index")
+      expect(pack instanceof Package).toBe true
+      expect(pack.metadata.name).toBe "package-with-index"
+
+    it "returns the package if it has an invalid keymap", ->
       spyOn(console, 'warn')
-      atom.config.set("core.disabledPackages", [])
-      expect(-> atom.packages.loadPackage("package-with-broken-keymap")).not.toThrow()
+      pack = atom.packages.loadPackage("package-with-broken-keymap")
+      expect(pack instanceof Package).toBe true
+      expect(pack.metadata.name).toBe "package-with-broken-keymap"
+
+    it "returns null if the package has an invalid package.json", ->
+      spyOn(console, 'warn')
+      expect(atom.packages.loadPackage("package-with-broken-package-json")).toBeNull()
+      expect(console.warn.callCount).toBe(1)
+      expect(console.warn.argsForCall[0][0]).toContain("Failed to load package.json")
+
+    it "returns null if the package is not found in any package directory", ->
+      spyOn(console, 'warn')
+      expect(atom.packages.loadPackage("this-package-cannot-be-found")).toBeNull()
+      expect(console.warn.callCount).toBe(1)
+      expect(console.warn.argsForCall[0][0]).toContain("Could not resolve")
 
   describe "::unloadPackage(name)", ->
     describe "when the package is active", ->
@@ -197,11 +214,30 @@ describe "PackageManager", ->
           runs ->
             expect(pack.mainModule.activate).toHaveBeenCalledWith({someNumber: 77})
 
-      it "logs warning instead of throwing an exception if the package fails to load", ->
-        atom.config.set("core.disabledPackages", [])
-        spyOn(console, "warn")
-        expect(-> atom.packages.activatePackage("package-that-throws-an-exception")).not.toThrow()
-        expect(console.warn).toHaveBeenCalled()
+      describe "when the package throws an error while loading", ->
+        it "logs a warning instead of throwing an exception", ->
+          atom.config.set("core.disabledPackages", [])
+          spyOn(console, "warn")
+          expect(-> atom.packages.activatePackage("package-that-throws-an-exception")).not.toThrow()
+          expect(console.warn).toHaveBeenCalled()
+
+      describe "when the package is not found", ->
+        it "rejects the promise", ->
+          atom.config.set("core.disabledPackages", [])
+
+          onSuccess = jasmine.createSpy('onSuccess')
+          onFailure = jasmine.createSpy('onFailure')
+          spyOn(console, 'warn')
+
+          atom.packages.activatePackage("this-doesnt-exist").then(onSuccess, onFailure)
+
+          waitsFor "promise to be rejected", ->
+            onFailure.callCount > 0
+
+          runs ->
+            expect(console.warn.callCount).toBe 1
+            expect(onFailure.mostRecentCall.args[0] instanceof Error).toBe true
+            expect(onFailure.mostRecentCall.args[0].message).toContain "Failed to load package 'this-doesnt-exist'"
 
       describe "keymap loading", ->
         describe "when the metadata does not contain a 'keymaps' manifest", ->
@@ -552,9 +588,9 @@ describe "PackageManager", ->
       themes = themeActivator.mostRecentCall.args[0]
       expect(['theme']).toContain(theme.getType()) for theme in themes
 
-  describe "::enablePackage() and ::disablePackage()", ->
+  describe "::enablePackage(id) and ::disablePackage(id)", ->
     describe "with packages", ->
-      it ".enablePackage() enables a disabled package", ->
+      it "enables a disabled package", ->
         packageName = 'package-with-main'
         atom.config.pushAtKeyPath('core.disabledPackages', packageName)
         atom.packages.observeDisabledPackages()
@@ -572,7 +608,7 @@ describe "PackageManager", ->
           expect(activatedPackages).toContain(pack)
           expect(atom.config.get('core.disabledPackages')).not.toContain packageName
 
-      it ".disablePackage() disables an enabled package", ->
+      it "disables an enabled package", ->
         packageName = 'package-with-main'
         waitsForPromise ->
           atom.packages.activatePackage(packageName)
@@ -587,6 +623,11 @@ describe "PackageManager", ->
           expect(activatedPackages).not.toContain(pack)
           expect(atom.config.get('core.disabledPackages')).toContain packageName
 
+      it "returns null if the package cannot be loaded", ->
+        spyOn(console, 'warn')
+        expect(atom.packages.enablePackage("this-doesnt-exist")).toBeNull()
+        expect(console.warn.callCount).toBe 1
+
     describe "with themes", ->
       reloadedHandler = null
 
@@ -597,7 +638,7 @@ describe "PackageManager", ->
       afterEach ->
         atom.themes.deactivateThemes()
 
-      it ".enablePackage() and .disablePackage() enables and disables a theme", ->
+      it "enables and disables a theme", ->
         packageName = 'theme-with-package-file'
 
         expect(atom.config.get('core.themes')).not.toContain packageName

spec/theme-manager-spec.coffee

@@ -131,7 +131,9 @@ describe "ThemeManager", ->
   describe "when a theme fails to load", ->
     it "logs a warning", ->
       spyOn(console, 'warn')
-      expect(-> atom.packages.activatePackage('a-theme-that-will-not-be-found')).toThrow()
+      atom.packages.activatePackage('a-theme-that-will-not-be-found')
+      expect(console.warn.callCount).toBe 1
+      expect(console.warn.argsForCall[0][0]).toContain "Could not resolve 'a-theme-that-will-not-be-found'"
 
   describe "::requireStylesheet(path)", ->
     beforeEach ->

src/config.coffee

@@ -882,13 +882,12 @@ class Config
 
   onDidChangeKeyPath: (keyPath, callback) ->
     oldValue = @get(keyPath)
-
-    didChange = =>
+    @emitter.on 'did-change', =>
       newValue = @get(keyPath)
-      callback({oldValue, newValue}) unless _.isEqual(oldValue, newValue)
-      oldValue = newValue
-
-    @emitter.on 'did-change', didChange
+      unless _.isEqual(oldValue, newValue)
+        event = {oldValue, newValue}
+        oldValue = newValue
+        callback(event)
 
   isSubKeyPath: (keyPath, subKeyPath) ->
     return false unless keyPath? and subKeyPath?
@@ -1003,12 +1002,12 @@ class Config
 
   onDidChangeScopedKeyPath: (scope, keyPath, callback) ->
     oldValue = @get(keyPath, {scope})
-    didChange = =>
+    @emitter.on 'did-change', =>
       newValue = @get(keyPath, {scope})
-      callback({oldValue, newValue}) unless _.isEqual(oldValue, newValue)
-      oldValue = newValue
-
-    @emitter.on 'did-change', didChange
+      unless _.isEqual(oldValue, newValue)
+        event = {oldValue, newValue}
+        oldValue = newValue
+        callback(event)
 
   # TODO: figure out how to change / remove this. The return value is awkward.
   # * language mode uses it for one thing.

src/package-manager.coffee

@@ -295,12 +295,12 @@ class PackageManager
           pack = new Package(packagePath, metadata)
         pack.load()
         @loadedPackages[pack.name] = pack
-        pack
+        return pack
       catch error
         console.warn "Failed to load package.json '#{path.basename(packagePath)}'", error.stack ? error
-
     else
-      throw new Error("Could not resolve '#{nameOrPath}' to a package path")
+      console.warn "Could not resolve '#{nameOrPath}' to a package path"
+    null
 
   unloadPackages: ->
     @unloadPackage(name) for name in _.keys(@loadedPackages)
@@ -337,11 +337,12 @@ class PackageManager
   activatePackage: (name) ->
     if pack = @getActivePackage(name)
       Q(pack)
-    else
-      pack = @loadPackage(name)
+    else if pack = @loadPackage(name)
       pack.activate().then =>
         @activePackages[pack.name] = pack
         pack
+    else
+      Q.reject(new Error("Failed to load package '#{name}'"))
 
   # Deactivate all packages
   deactivatePackages: ->