From 8ba098b6407934803f92786fd5176fd61e5f9aaa Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 14:38:15 -0800 Subject: [PATCH 01/28] Throw an error when adding an invalid selector --- spec/command-registry-spec.coffee | 10 ++++++++++ src/command-registry.coffee | 16 ++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/spec/command-registry-spec.coffee b/spec/command-registry-spec.coffee index 2af185b16..b92d3e6b0 100644 --- a/spec/command-registry-spec.coffee +++ b/spec/command-registry-spec.coffee @@ -148,6 +148,16 @@ describe "CommandRegistry", -> grandchild.dispatchEvent(new CustomEvent('command-2', bubbles: true)) expect(calls).toEqual [] + describe "::add(selector, commandName, callback)", -> + it "throws an error when called with an invalid selector", -> + badSelector = '<>' + addError = null + try + registry.add badSelector, 'foo:bar', -> + catch error + addError = error + expect(addError.message).toContain(badSelector) + describe "::findCommands({target})", -> it "returns commands that can be invoked on the target or its ancestors", -> registry.add '.parent', 'namespace:command-1', -> diff --git a/src/command-registry.coffee b/src/command-registry.coffee index 71e35f7e5..8d4b86dd5 100644 --- a/src/command-registry.coffee +++ b/src/command-registry.coffee @@ -87,6 +87,8 @@ class CommandRegistry return disposable if typeof target is 'string' + unless @isSelectorValid(target) + throw new Error("'#{target}' is not a valid selector") @addSelectorBasedListener(target, commandName, callback) else @addInlineListener(target, commandName, callback) @@ -235,6 +237,20 @@ class CommandRegistry window.addEventListener(commandName, @handleCommandEvent, true) @registeredCommands[commandName] = true + isSelectorValid: (selector) -> + @selectorCache ?= {} + cachedValue = @selectorCache[selector] + return cachedValue if cachedValue? + + @testElement ?= document.createElement('div') + try + @testElement.webkitMatchesSelector(selector) + @selectorCache[selector] = true + true + catch selectorError + @selectorCache[selector] = false + false + class SelectorBasedListener constructor: (@selector, @callback) -> @specificity = (SpecificityCache[@selector] ?= specificity(@selector)) From 333a495d55493d093452266e34ec6395c15a5e97 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 14:38:30 -0800 Subject: [PATCH 02/28] Catch errors adding activation commands --- .../package.json | 9 +++++++++ .../packages/package-with-invalid-selectors/package.json | 9 +++++++++ spec/package-manager-spec.coffee | 5 +++++ src/package.coffee | 5 ++++- 4 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 spec/fixtures/packages/package-with-invalid-activation-commands/package.json create mode 100644 spec/fixtures/packages/package-with-invalid-selectors/package.json diff --git a/spec/fixtures/packages/package-with-invalid-activation-commands/package.json b/spec/fixtures/packages/package-with-invalid-activation-commands/package.json new file mode 100644 index 000000000..d98b7ef92 --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-activation-commands/package.json @@ -0,0 +1,9 @@ +{ + "name": "package-with-invalid-selectors", + "version": "1.0.0", + "activationCommands": { + "<>": [ + "foo:bar" + ] + } +} diff --git a/spec/fixtures/packages/package-with-invalid-selectors/package.json b/spec/fixtures/packages/package-with-invalid-selectors/package.json new file mode 100644 index 000000000..d98b7ef92 --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-selectors/package.json @@ -0,0 +1,9 @@ +{ + "name": "package-with-invalid-selectors", + "version": "1.0.0", + "activationCommands": { + "<>": [ + "foo:bar" + ] + } +} diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index d9898505c..4d6c56d4e 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -212,6 +212,11 @@ describe "PackageManager", -> runs -> expect(mainModule.activate.callCount).toBe 1 + it "logs a warning when the activation commands are invalid", -> + spyOn(console, 'warn') + expect(-> atom.packages.activatePackage('package-with-invalid-activation-commands')).not.toThrow() + expect(console.warn.callCount).toBe(1) + describe "when the package has no main module", -> it "does not throw an exception", -> spyOn(console, "error") diff --git a/src/package.coffee b/src/package.coffee index d8ba4a1d8..e47b0038a 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -146,7 +146,10 @@ class Package @measure 'activateTime', => @activateResources() if @hasActivationCommands() - @subscribeToActivationCommands() + try + @subscribeToActivationCommands() + catch e + console.warn "Failed to subscribe to activation commands for package '#{@name}'", e.stack else @activateNow() From 2a12f7779d8c4abc3b138335d4fe6493db6b41dc Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 14:46:35 -0800 Subject: [PATCH 03/28] Add selector parser helper class --- src/command-registry.coffee | 17 ++--------------- src/selector-parser.coffee | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 15 deletions(-) create mode 100644 src/selector-parser.coffee diff --git a/src/command-registry.coffee b/src/command-registry.coffee index 8d4b86dd5..ac77bcc27 100644 --- a/src/command-registry.coffee +++ b/src/command-registry.coffee @@ -2,6 +2,7 @@ {specificity} = require 'clear-cut' _ = require 'underscore-plus' {$} = require './space-pen-extensions' +{isSelectorValid} = require './selector-parser' SequenceCount = 0 SpecificityCache = {} @@ -87,7 +88,7 @@ class CommandRegistry return disposable if typeof target is 'string' - unless @isSelectorValid(target) + unless isSelectorValid(target) throw new Error("'#{target}' is not a valid selector") @addSelectorBasedListener(target, commandName, callback) else @@ -237,20 +238,6 @@ class CommandRegistry window.addEventListener(commandName, @handleCommandEvent, true) @registeredCommands[commandName] = true - isSelectorValid: (selector) -> - @selectorCache ?= {} - cachedValue = @selectorCache[selector] - return cachedValue if cachedValue? - - @testElement ?= document.createElement('div') - try - @testElement.webkitMatchesSelector(selector) - @selectorCache[selector] = true - true - catch selectorError - @selectorCache[selector] = false - false - class SelectorBasedListener constructor: (@selector, @callback) -> @specificity = (SpecificityCache[@selector] ?= specificity(@selector)) diff --git a/src/selector-parser.coffee b/src/selector-parser.coffee new file mode 100644 index 000000000..fc3744a1f --- /dev/null +++ b/src/selector-parser.coffee @@ -0,0 +1,16 @@ +selectorCache = null +testElement = null + +exports.isSelectorValid = (selector) -> + selectorCache ?= {} + cachedValue = selectorCache[selector] + return cachedValue if cachedValue? + + testElement ?= document.createElement('div') + try + testElement.webkitMatchesSelector(selector) + selectorCache[selector] = true + true + catch selectorError + selectorCache[selector] = false + false From 5902bc42e983289aa0fd080940b19b81e2636ca1 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 14:52:18 -0800 Subject: [PATCH 04/28] Throw error when adding context menu with invalid selector --- spec/context-menu-manager-spec.coffee | 8 ++++++++ src/context-menu-manager.coffee | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/spec/context-menu-manager-spec.coffee b/spec/context-menu-manager-spec.coffee index 1c3c1f21a..ab1f05080 100644 --- a/spec/context-menu-manager-spec.coffee +++ b/spec/context-menu-manager-spec.coffee @@ -151,6 +151,14 @@ describe "ContextMenuManager", -> shouldDisplay = false expect(contextMenu.templateForEvent(dispatchedEvent)).toEqual [] + it "throws an error when the selector is invalid", -> + addError = null + try + contextMenu.add '<>': [{label: 'A', command: 'a'}] + catch error + addError = error + expect(addError.message).toContain('<>') + describe "when the menus are specified in a legacy format", -> beforeEach -> jasmine.snapshotDeprecations() diff --git a/src/context-menu-manager.coffee b/src/context-menu-manager.coffee index b890fc29a..f6d2d7f96 100644 --- a/src/context-menu-manager.coffee +++ b/src/context-menu-manager.coffee @@ -8,6 +8,7 @@ fs = require 'fs-plus' {Disposable} = require 'event-kit' Grim = require 'grim' MenuHelpers = require './menu-helpers' +{isSelectorValid} = require './selector-parser' SpecificityCache = {} @@ -123,6 +124,9 @@ class ContextMenuManager addedItemSets = [] for selector, items of itemsBySelector + unless isSelectorValid(selector) + throw new Error("'#{selector}' is not a valid selector") + itemSet = new ContextMenuItemSet(selector, items) addedItemSets.push(itemSet) @itemSets.push(itemSet) From a7bd20f08f3728ed41e388ebe3df9273bc29fbdb Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 14:53:50 -0800 Subject: [PATCH 05/28] Remove unused fixture --- .../packages/package-with-invalid-selectors/package.json | 9 --------- 1 file changed, 9 deletions(-) delete mode 100644 spec/fixtures/packages/package-with-invalid-selectors/package.json diff --git a/spec/fixtures/packages/package-with-invalid-selectors/package.json b/spec/fixtures/packages/package-with-invalid-selectors/package.json deleted file mode 100644 index d98b7ef92..000000000 --- a/spec/fixtures/packages/package-with-invalid-selectors/package.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "name": "package-with-invalid-selectors", - "version": "1.0.0", - "activationCommands": { - "<>": [ - "foo:bar" - ] - } -} From c172f78ab54fd8d399e1d472000d0bb70d706e0e Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:06:38 -0800 Subject: [PATCH 06/28] Use consistent selector error --- src/command-registry.coffee | 5 ++--- src/context-menu-manager.coffee | 5 ++--- src/selector-parser.coffee | 7 +++++++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/command-registry.coffee b/src/command-registry.coffee index ac77bcc27..07ebcd812 100644 --- a/src/command-registry.coffee +++ b/src/command-registry.coffee @@ -2,7 +2,7 @@ {specificity} = require 'clear-cut' _ = require 'underscore-plus' {$} = require './space-pen-extensions' -{isSelectorValid} = require './selector-parser' +{validateSelector} = require './selector-parser' SequenceCount = 0 SpecificityCache = {} @@ -88,8 +88,7 @@ class CommandRegistry return disposable if typeof target is 'string' - unless isSelectorValid(target) - throw new Error("'#{target}' is not a valid selector") + validateSelector(target) @addSelectorBasedListener(target, commandName, callback) else @addInlineListener(target, commandName, callback) diff --git a/src/context-menu-manager.coffee b/src/context-menu-manager.coffee index f6d2d7f96..0cc56079b 100644 --- a/src/context-menu-manager.coffee +++ b/src/context-menu-manager.coffee @@ -8,7 +8,7 @@ fs = require 'fs-plus' {Disposable} = require 'event-kit' Grim = require 'grim' MenuHelpers = require './menu-helpers' -{isSelectorValid} = require './selector-parser' +{validateSelector} = require './selector-parser' SpecificityCache = {} @@ -124,8 +124,7 @@ class ContextMenuManager addedItemSets = [] for selector, items of itemsBySelector - unless isSelectorValid(selector) - throw new Error("'#{selector}' is not a valid selector") + validateSelector(selector) itemSet = new ContextMenuItemSet(selector, items) addedItemSets.push(itemSet) diff --git a/src/selector-parser.coffee b/src/selector-parser.coffee index fc3744a1f..ddb7c337a 100644 --- a/src/selector-parser.coffee +++ b/src/selector-parser.coffee @@ -1,6 +1,13 @@ selectorCache = null testElement = null +exports.validateSelector = (selector) -> + return if exports.isSelectorValid(selector) + + error = new Error("'#{selector}' is not a valid selector") + error.code = 'EBADSELECTOR' + throw error + exports.isSelectorValid = (selector) -> selectorCache ?= {} cachedValue = selectorCache[selector] From 8ba434f4c42604a369aa089ea1885874ee9f9ff5 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:08:10 -0800 Subject: [PATCH 07/28] Show notifications on load/activate errors --- src/package.coffee | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/package.coffee b/src/package.coffee index e47b0038a..530a06c2b 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -126,9 +126,8 @@ class Package @loadStylesheets() @settingsPromise = @loadSettings() @requireMainModule() unless @hasActivationCommands() - catch error - console.warn "Failed to load package named '#{@name}'", error.stack ? error + @handleError("Failed to load the #{@name} package", error) this reset: -> @@ -148,8 +147,12 @@ class Package if @hasActivationCommands() try @subscribeToActivationCommands() - catch e - console.warn "Failed to subscribe to activation commands for package '#{@name}'", e.stack + catch error + if error.code is 'EBADSELECTOR' + metadataPath = path.join(@path, 'package.json') + error.message += " in #{metadataPath}" + error.stack += "\n at #{metadataPath}:1:1" + @handleError("Failed to activate the #{@name} package", error) else @activateNow() @@ -163,8 +166,8 @@ class Package @mainModule.activate?(atom.packages.getPackageState(@name) ? {}) @mainActivated = true @activateServices() - catch e - console.warn "Failed to activate package named '#{@name}'", e.stack + catch error + @handleError("Failed to activate the #{@name} package", error) @activationDeferred?.resolve() @@ -531,3 +534,18 @@ class Package @compatible = @incompatibleModules.length is 0 else @compatible = true + + handleError: (message, error) -> + if error.filename and error.location and (error instanceof SyntaxError) + location = "#{error.filename}:#{error.location.first_line + 1}:#{error.location.first_column + 1}" + detail = "#{error.message} in #{location}" + stack = """ + SyntaxError: #{error.message} + at #{location} + """ + else + detail = error.message + stack = error.stack ? error + + console.trace() + atom.notifications.addFatalError(message, {stack, detail, dismissable: true}) From 33e01256a7c6f56dea0c68cb042f62c96762da35 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:08:30 -0800 Subject: [PATCH 08/28] Only manipulate stack for selector errors --- src/package.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/package.coffee b/src/package.coffee index 530a06c2b..07726aa0a 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -151,7 +151,7 @@ class Package if error.code is 'EBADSELECTOR' metadataPath = path.join(@path, 'package.json') error.message += " in #{metadataPath}" - error.stack += "\n at #{metadataPath}:1:1" + error.stack += "\n at #{metadataPath}:1:1" @handleError("Failed to activate the #{@name} package", error) else @activateNow() From cb0bf287935b71528bf1d8dd331f2868f4348003 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:21:53 -0800 Subject: [PATCH 09/28] Remove stray trace --- src/package.coffee | 1 - 1 file changed, 1 deletion(-) diff --git a/src/package.coffee b/src/package.coffee index 07726aa0a..6d1e674b3 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -547,5 +547,4 @@ class Package detail = error.message stack = error.stack ? error - console.trace() atom.notifications.addFatalError(message, {stack, detail, dismissable: true}) From fa2eab5b7ef379204f9da7b97cb0242b0415e60a Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:28:48 -0800 Subject: [PATCH 10/28] Show notification when parsing package.json fails --- spec/package-manager-spec.coffee | 8 ++++---- src/package-manager.coffee | 24 +++++++++++++++--------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index 4d6c56d4e..bab5bb260 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -18,7 +18,6 @@ describe "PackageManager", -> expect(pack.metadata.name).toBe "package-with-index" it "returns the package if it has an invalid keymap", -> - spyOn(console, 'warn') pack = atom.packages.loadPackage("package-with-broken-keymap") expect(pack instanceof Package).toBe true expect(pack.metadata.name).toBe "package-with-broken-keymap" @@ -30,10 +29,11 @@ describe "PackageManager", -> expect(pack.stylesheets.length).toBe 0 it "returns null if the package has an invalid package.json", -> - spyOn(console, 'warn') + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) 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") + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load the package-with-broken-package-json package") it "returns null if the package is not found in any package directory", -> spyOn(console, 'warn') diff --git a/src/package-manager.coffee b/src/package-manager.coffee index bcd7dc28e..22c62e9b4 100644 --- a/src/package-manager.coffee +++ b/src/package-manager.coffee @@ -343,16 +343,22 @@ class PackageManager try metadata = Package.loadMetadata(packagePath) ? {} - if metadata.theme - pack = new ThemePackage(packagePath, metadata) - else - pack = new Package(packagePath, metadata) - pack.load() - @loadedPackages[pack.name] = pack - @emitter.emit 'did-load-package', pack - return pack catch error - console.warn "Failed to load package.json '#{path.basename(packagePath)}'", error.stack ? error + metadataPath = path.join(packagePath, 'package.json') + detail = error.message + " in #{metadataPath}" + stack = error.stack + "\n at #{metadataPath}:1:1" + message = "Failed to load the #{path.basename(packagePath)} package" + atom.notifications.addFatalError(message, {stack, detail, dismissable: true}) + return null + + if metadata.theme + pack = new ThemePackage(packagePath, metadata) + else + pack = new Package(packagePath, metadata) + pack.load() + @loadedPackages[pack.name] = pack + @emitter.emit 'did-load-package', pack + return pack else console.warn "Could not resolve '#{nameOrPath}' to a package path" null From 6c87dc05f3a0866d0ec6b38770a70985b9c6fddf Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:33:58 -0800 Subject: [PATCH 11/28] Migrate specs from console.warn to notifications --- spec/package-manager-spec.coffee | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index bab5bb260..58392ccbc 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -213,9 +213,11 @@ describe "PackageManager", -> expect(mainModule.activate.callCount).toBe 1 it "logs a warning when the activation commands are invalid", -> - spyOn(console, 'warn') + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) expect(-> atom.packages.activatePackage('package-with-invalid-activation-commands')).not.toThrow() - expect(console.warn.callCount).toBe(1) + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to activate the package-with-invalid-activation-commands package") describe "when the package has no main module", -> it "does not throw an exception", -> @@ -262,11 +264,14 @@ describe "PackageManager", -> runs -> expect(activatedPackage.name).toBe 'package-with-main' describe "when the package throws an error while loading", -> - it "logs a warning instead of throwing an exception", -> + fit "adds a notification instead of throwing an exception", -> atom.config.set("core.disabledPackages", []) - spyOn(console, "warn") + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) expect(-> atom.packages.activatePackage("package-that-throws-an-exception")).not.toThrow() - expect(console.warn).toHaveBeenCalled() + expect(addErrorHandler.callCount).toBe 2 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load the package-that-throws-an-exception package") + expect(addErrorHandler.argsForCall[1][0].message).toContain("Failed to activate the package-that-throws-an-exception package") describe "when the package is not found", -> it "rejects the promise", -> From 628380ff3dd5c091374cc95338df2470604e619b Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:35:58 -0800 Subject: [PATCH 12/28] Unfocus spec --- spec/package-manager-spec.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index 58392ccbc..773f8ffa5 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -264,7 +264,7 @@ describe "PackageManager", -> runs -> expect(activatedPackage.name).toBe 'package-with-main' describe "when the package throws an error while loading", -> - fit "adds a notification instead of throwing an exception", -> + it "adds a notification instead of throwing an exception", -> atom.config.set("core.disabledPackages", []) addErrorHandler = jasmine.createSpy() atom.notifications.onDidAddNotification(addErrorHandler) From a2d9ba2d2e7f1ff14b820dd8520917b34a1943e6 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:40:39 -0800 Subject: [PATCH 13/28] Only try to require main module once --- spec/package-manager-spec.coffee | 5 ++--- src/package.coffee | 6 ++++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index 773f8ffa5..5855006dd 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -2,7 +2,7 @@ Package = require '../src/package' {Disposable} = require 'atom' -describe "PackageManager", -> +fdescribe "PackageManager", -> workspaceElement = null beforeEach -> @@ -269,9 +269,8 @@ describe "PackageManager", -> addErrorHandler = jasmine.createSpy() atom.notifications.onDidAddNotification(addErrorHandler) expect(-> atom.packages.activatePackage("package-that-throws-an-exception")).not.toThrow() - expect(addErrorHandler.callCount).toBe 2 + expect(addErrorHandler.callCount).toBe 1 expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load the package-that-throws-an-exception package") - expect(addErrorHandler.argsForCall[1][0].message).toContain("Failed to activate the package-that-throws-an-exception package") describe "when the package is not found", -> it "rejects the promise", -> diff --git a/src/package.coffee b/src/package.coffee index 6d1e674b3..ef4cc9c42 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -376,7 +376,7 @@ class Package @activateStylesheets() requireMainModule: -> - return @mainModule if @mainModule? + return @mainModule if @mainModuleRequired unless @isCompatible() console.warn """ Failed to require the main module of '#{@name}' because it requires an incompatible native module. @@ -384,7 +384,9 @@ class Package """ return mainModulePath = @getMainModulePath() - @mainModule = require(mainModulePath) if fs.isFileSync(mainModulePath) + if fs.isFileSync(mainModulePath) + @mainModuleRequired = true + @mainModule = require(mainModulePath) getMainModulePath: -> return @mainModulePath if @resolvedMainModulePath From 59c3dea77bcdd8d67cc030fb85621bb8e6e071d9 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:51:38 -0800 Subject: [PATCH 14/28] Show notification for invalid context menu selector --- .../menus/menu.json | 10 +++++ .../package.json | 4 ++ spec/package-manager-spec.coffee | 11 +++++- src/package.coffee | 39 ++++++++++++------- 4 files changed, 49 insertions(+), 15 deletions(-) create mode 100644 spec/fixtures/packages/package-with-invalid-context-menu/menus/menu.json create mode 100644 spec/fixtures/packages/package-with-invalid-context-menu/package.json diff --git a/spec/fixtures/packages/package-with-invalid-context-menu/menus/menu.json b/spec/fixtures/packages/package-with-invalid-context-menu/menus/menu.json new file mode 100644 index 000000000..67197f8fe --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-context-menu/menus/menu.json @@ -0,0 +1,10 @@ +{ + "context-menu": { + "<>": [ + { + "label": "Hello", + "command:": "world" + } + ] + } +} diff --git a/spec/fixtures/packages/package-with-invalid-context-menu/package.json b/spec/fixtures/packages/package-with-invalid-context-menu/package.json new file mode 100644 index 000000000..c742d422a --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-context-menu/package.json @@ -0,0 +1,4 @@ +{ + "name": "package-with-invalid-context-menu", + "version": "1.0.0" +} diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index 5855006dd..a021f979a 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -2,7 +2,7 @@ Package = require '../src/package' {Disposable} = require 'atom' -fdescribe "PackageManager", -> +describe "PackageManager", -> workspaceElement = null beforeEach -> @@ -212,13 +212,20 @@ fdescribe "PackageManager", -> runs -> expect(mainModule.activate.callCount).toBe 1 - it "logs a warning when the activation commands are invalid", -> + it "adds a notification when the activation commands are invalid", -> addErrorHandler = jasmine.createSpy() atom.notifications.onDidAddNotification(addErrorHandler) expect(-> atom.packages.activatePackage('package-with-invalid-activation-commands')).not.toThrow() expect(addErrorHandler.callCount).toBe 1 expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to activate the package-with-invalid-activation-commands package") + it "adds a notification when the context menu is invalid", -> + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) + expect(-> atom.packages.activatePackage('package-with-invalid-context-menu')).not.toThrow() + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to activate the package-with-invalid-context-menu package") + describe "when the package has no main module", -> it "does not throw an exception", -> spyOn(console, "error") diff --git a/src/package.coffee b/src/package.coffee index ef4cc9c42..148e5d4e4 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -143,18 +143,14 @@ class Package unless @activationDeferred? @activationDeferred = Q.defer() @measure 'activateTime', => - @activateResources() - if @hasActivationCommands() - try + try + @activateResources() + if @hasActivationCommands() @subscribeToActivationCommands() - catch error - if error.code is 'EBADSELECTOR' - metadataPath = path.join(@path, 'package.json') - error.message += " in #{metadataPath}" - error.stack += "\n at #{metadataPath}:1:1" - @handleError("Failed to activate the #{@name} package", error) - else - @activateNow() + else + @activateNow() + catch error + @handleError("Failed to activate the #{@name} package", error) Q.all([@grammarsPromise, @settingsPromise, @activationDeferred.promise]) @@ -206,7 +202,16 @@ class Package activateResources: -> @activationDisposables = new CompositeDisposable @activationDisposables.add(atom.keymaps.add(keymapPath, map)) for [keymapPath, map] in @keymaps - @activationDisposables.add(atom.contextMenu.add(map['context-menu'])) for [menuPath, map] in @menus when map['context-menu']? + + for [menuPath, map] in @menus when map['context-menu']? + try + @activationDisposables.add(atom.contextMenu.add(map['context-menu'])) + catch error + if error.code is 'EBADSELECTOR' + error.message += " in #{menuPath}" + error.stack += "\n at #{menuPath}:1:1" + throw error + @activationDisposables.add(atom.menu.add(map['menu'])) for [menuPath, map] in @menus when map['menu']? unless @grammarsActivated @@ -417,7 +422,15 @@ class Package do (selector, command) => # Add dummy command so it appears in menu. # The real command will be registered on package activation - @activationCommandSubscriptions.add atom.commands.add selector, command, -> + try + @activationCommandSubscriptions.add atom.commands.add selector, command, -> + catch error + if error.code is 'EBADSELECTOR' + metadataPath = path.join(@path, 'package.json') + error.message += " in #{metadataPath}" + error.stack += "\n at #{metadataPath}:1:1" + throw error + @activationCommandSubscriptions.add atom.commands.onWillDispatch (event) => return unless event.type is command currentTarget = event.target From 1bb011deed3bb23ddf3573db4732970fb2b30ea4 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:55:37 -0800 Subject: [PATCH 15/28] :art: --- src/context-menu-manager.coffee | 1 - 1 file changed, 1 deletion(-) diff --git a/src/context-menu-manager.coffee b/src/context-menu-manager.coffee index 0cc56079b..56f85199f 100644 --- a/src/context-menu-manager.coffee +++ b/src/context-menu-manager.coffee @@ -125,7 +125,6 @@ class ContextMenuManager for selector, items of itemsBySelector validateSelector(selector) - itemSet = new ContextMenuItemSet(selector, items) addedItemSets.push(itemSet) @itemSets.push(itemSet) From 7b38750b95e0925b6681445f5d859a0a7d7969d4 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:57:25 -0800 Subject: [PATCH 16/28] :memo: Doc selector parser --- src/selector-parser.coffee | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/selector-parser.coffee b/src/selector-parser.coffee index ddb7c337a..6b2afffc1 100644 --- a/src/selector-parser.coffee +++ b/src/selector-parser.coffee @@ -1,13 +1,8 @@ selectorCache = null testElement = null -exports.validateSelector = (selector) -> - return if exports.isSelectorValid(selector) - - error = new Error("'#{selector}' is not a valid selector") - error.code = 'EBADSELECTOR' - throw error - +# Parses CSS selectors and memoizes their validity so each selector will only +# be parsed once. exports.isSelectorValid = (selector) -> selectorCache ?= {} cachedValue = selectorCache[selector] @@ -21,3 +16,11 @@ exports.isSelectorValid = (selector) -> catch selectorError selectorCache[selector] = false false + +# Parse the given selector and throw an error if it is invalid +exports.validateSelector = (selector) -> + return if exports.isSelectorValid(selector) + + error = new Error("'#{selector}' is not a valid selector") + error.code = 'EBADSELECTOR' + throw error From 346e66960158a25ff594fe260838a3fc6cdc5760 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 16:59:36 -0800 Subject: [PATCH 17/28] :memo: Mention css --- src/selector-parser.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/selector-parser.coffee b/src/selector-parser.coffee index 6b2afffc1..df8ff0350 100644 --- a/src/selector-parser.coffee +++ b/src/selector-parser.coffee @@ -17,7 +17,7 @@ exports.isSelectorValid = (selector) -> selectorCache[selector] = false false -# Parse the given selector and throw an error if it is invalid +# Parse the given CSS selector and throw an error if it is invalid. exports.validateSelector = (selector) -> return if exports.isSelectorValid(selector) From f611e235e9002b02eb6df797beae51c61a934e05 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 17:00:55 -0800 Subject: [PATCH 18/28] Extract notification helper --- src/package-manager.coffee | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/package-manager.coffee b/src/package-manager.coffee index 22c62e9b4..58cade753 100644 --- a/src/package-manager.coffee +++ b/src/package-manager.coffee @@ -344,11 +344,7 @@ class PackageManager try metadata = Package.loadMetadata(packagePath) ? {} catch error - metadataPath = path.join(packagePath, 'package.json') - detail = error.message + " in #{metadataPath}" - stack = error.stack + "\n at #{metadataPath}:1:1" - message = "Failed to load the #{path.basename(packagePath)} package" - atom.notifications.addFatalError(message, {stack, detail, dismissable: true}) + @handleMetadataError(error, packagePath) return null if metadata.theme @@ -427,3 +423,10 @@ class PackageManager pack.deactivate() delete @activePackages[pack.name] @emitter.emit 'did-deactivate-package', pack + + handleMetadataError: (error, packagePath)-> + metadataPath = path.join(packagePath, 'package.json') + detail = error.message + " in #{metadataPath}" + stack = error.stack + "\n at #{metadataPath}:1:1" + message = "Failed to load the #{path.basename(packagePath)} package" + atom.notifications.addFatalError(message, {stack, detail, dismissable: true}) From 71c138397a8bdb94eec9eeca765b87cbb37d5785 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 17:11:08 -0800 Subject: [PATCH 19/28] Remove lint --- src/package-manager.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/package-manager.coffee b/src/package-manager.coffee index 58cade753..c663d6064 100644 --- a/src/package-manager.coffee +++ b/src/package-manager.coffee @@ -424,7 +424,7 @@ class PackageManager delete @activePackages[pack.name] @emitter.emit 'did-deactivate-package', pack - handleMetadataError: (error, packagePath)-> + handleMetadataError: (error, packagePath) -> metadataPath = path.join(packagePath, 'package.json') detail = error.message + " in #{metadataPath}" stack = error.stack + "\n at #{metadataPath}:1:1" From cec82bd6ca30b20980893981ee2addf058124ea2 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 17:26:25 -0800 Subject: [PATCH 20/28] Don't use fatal error for package.json errors Notifications can't associate it with a project path since the project never loads --- src/package-manager.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/package-manager.coffee b/src/package-manager.coffee index c663d6064..355c65a78 100644 --- a/src/package-manager.coffee +++ b/src/package-manager.coffee @@ -429,4 +429,4 @@ class PackageManager detail = error.message + " in #{metadataPath}" stack = error.stack + "\n at #{metadataPath}:1:1" message = "Failed to load the #{path.basename(packagePath)} package" - atom.notifications.addFatalError(message, {stack, detail, dismissable: true}) + atom.notifications.addError(message, {stack, detail, dismissable: true}) From ce33e63532151c28fc0ceecaabc86c52d011e4f8 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 3 Mar 2015 18:07:08 -0800 Subject: [PATCH 21/28] selector-parser -> selector-validator --- src/command-registry.coffee | 2 +- src/context-menu-manager.coffee | 2 +- src/{selector-parser.coffee => selector-validator.coffee} | 0 3 files changed, 2 insertions(+), 2 deletions(-) rename src/{selector-parser.coffee => selector-validator.coffee} (100%) diff --git a/src/command-registry.coffee b/src/command-registry.coffee index 07ebcd812..ead4415d2 100644 --- a/src/command-registry.coffee +++ b/src/command-registry.coffee @@ -2,7 +2,7 @@ {specificity} = require 'clear-cut' _ = require 'underscore-plus' {$} = require './space-pen-extensions' -{validateSelector} = require './selector-parser' +{validateSelector} = require './selector-validator' SequenceCount = 0 SpecificityCache = {} diff --git a/src/context-menu-manager.coffee b/src/context-menu-manager.coffee index 56f85199f..3f281afb1 100644 --- a/src/context-menu-manager.coffee +++ b/src/context-menu-manager.coffee @@ -8,7 +8,7 @@ fs = require 'fs-plus' {Disposable} = require 'event-kit' Grim = require 'grim' MenuHelpers = require './menu-helpers' -{validateSelector} = require './selector-parser' +{validateSelector} = require './selector-validator' SpecificityCache = {} diff --git a/src/selector-parser.coffee b/src/selector-validator.coffee similarity index 100% rename from src/selector-parser.coffee rename to src/selector-validator.coffee From 2f476b31ecdb21135ca0456be43e68b47cf47883 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Mar 2015 10:10:43 -0800 Subject: [PATCH 22/28] Don't return collected array --- src/package.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/src/package.coffee b/src/package.coffee index 148e5d4e4..73d4708c0 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -229,6 +229,7 @@ class Package for name, {versions} of @metadata.consumedServices for version, methodName of versions @activationDisposables.add atom.packages.serviceHub.consume(name, version, @mainModule[methodName].bind(@mainModule)) + return loadKeymaps: -> if @bundledPackage and packagesCache[@name]? From bf9b51724010486c4f59bddc5784def41ea4c955 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Mar 2015 10:33:45 -0800 Subject: [PATCH 23/28] :racehorse: webkitMatchesSelector -> querySelector --- src/selector-validator.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/selector-validator.coffee b/src/selector-validator.coffee index df8ff0350..f8dcb240a 100644 --- a/src/selector-validator.coffee +++ b/src/selector-validator.coffee @@ -10,7 +10,9 @@ exports.isSelectorValid = (selector) -> testElement ?= document.createElement('div') try - testElement.webkitMatchesSelector(selector) + # querySelector appears to be faster than webkitMatchesSelector + # http://jsperf.com/query-vs-matches + testElement.querySelector(selector) selectorCache[selector] = true true catch selectorError From f10d478c7a035a5f1dc52af3d80af2e4cc5e5c5b Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Mar 2015 10:43:40 -0800 Subject: [PATCH 24/28] :art: --- src/package-manager.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/package-manager.coffee b/src/package-manager.coffee index 355c65a78..9f53d45e2 100644 --- a/src/package-manager.coffee +++ b/src/package-manager.coffee @@ -426,7 +426,7 @@ class PackageManager handleMetadataError: (error, packagePath) -> metadataPath = path.join(packagePath, 'package.json') - detail = error.message + " in #{metadataPath}" - stack = error.stack + "\n at #{metadataPath}:1:1" + detail = "#{error.message} in #{metadataPath}" + stack = "#{error.stack}\n at #{metadataPath}:1:1" message = "Failed to load the #{path.basename(packagePath)} package" atom.notifications.addError(message, {stack, detail, dismissable: true}) From 49a26ea326010ed43ef47dbbdb738fa0956a8fd4 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Mar 2015 10:51:03 -0800 Subject: [PATCH 25/28] Add notification for package settings errors --- src/package.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/package.coffee b/src/package.coffee index 73d4708c0..fded24f9b 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -321,7 +321,9 @@ class Package loadSettingsFile = (settingsPath, callback) => ScopedProperties.load settingsPath, (error, settings) => if error? - console.warn("Failed to load package settings: #{settingsPath}", error.stack ? error) + detail = "#{error.message} in #{settingsPath}" + stack += "#{error.stack}\n at #{settingsPath}:1:1" + atom.notifications.addFatalError("Failed to load the #{@name} package settings", {stack, detail, dismissable: true}) else @settings.push(settings) settings.activate() if @settingsActivated From 92754bb9a3560191bb26f4b7e5bda87ee13ce531 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Mar 2015 10:51:49 -0800 Subject: [PATCH 26/28] Add notification for package grammar errors --- src/package.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/package.coffee b/src/package.coffee index fded24f9b..4785a7098 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -302,7 +302,9 @@ class Package loadGrammar = (grammarPath, callback) => atom.grammars.readGrammar grammarPath, (error, grammar) => if error? - console.warn("Failed to load grammar: #{grammarPath}", error.stack ? error) + detail = "#{error.message} in #{grammarPath}" + stack += "#{error.stack}\n at #{grammarPath}:1:1" + atom.notifications.addFatalError("Failed to load a #{@name} package grammar", {stack, detail, dismissable: true}) else grammar.packageName = @name @grammars.push(grammar) From f02fa4a245b25e539a77f8391b67d2839ef00106 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Mar 2015 11:12:09 -0800 Subject: [PATCH 27/28] Add spec for invalid grammar notification --- .../grammars/grammar.json | 1 + .../package-with-invalid-grammar/package.json | 4 ++++ spec/package-manager-spec.coffee | 13 +++++++++++++ src/package.coffee | 2 +- 4 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 spec/fixtures/packages/package-with-invalid-grammar/grammars/grammar.json create mode 100644 spec/fixtures/packages/package-with-invalid-grammar/package.json diff --git a/spec/fixtures/packages/package-with-invalid-grammar/grammars/grammar.json b/spec/fixtures/packages/package-with-invalid-grammar/grammars/grammar.json new file mode 100644 index 000000000..41ec4c527 --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-grammar/grammars/grammar.json @@ -0,0 +1 @@ +>< diff --git a/spec/fixtures/packages/package-with-invalid-grammar/package.json b/spec/fixtures/packages/package-with-invalid-grammar/package.json new file mode 100644 index 000000000..4ed4081f5 --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-grammar/package.json @@ -0,0 +1,4 @@ +{ + "name": "package-with-invalid-grammar", + "version": "1.0.0" +} diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index a021f979a..76adfe847 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -226,6 +226,19 @@ describe "PackageManager", -> expect(addErrorHandler.callCount).toBe 1 expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to activate the package-with-invalid-context-menu package") + it "adds a notification when the grammar is invalid", -> + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) + + expect(-> atom.packages.activatePackage('package-with-invalid-grammar')).not.toThrow() + + waitsFor -> + addErrorHandler.callCount > 0 + + runs -> + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load a package-with-invalid-grammar package grammar") + describe "when the package has no main module", -> it "does not throw an exception", -> spyOn(console, "error") diff --git a/src/package.coffee b/src/package.coffee index 4785a7098..0c4f7319d 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -303,7 +303,7 @@ class Package atom.grammars.readGrammar grammarPath, (error, grammar) => if error? detail = "#{error.message} in #{grammarPath}" - stack += "#{error.stack}\n at #{grammarPath}:1:1" + stack = "#{error.stack}\n at #{grammarPath}:1:1" atom.notifications.addFatalError("Failed to load a #{@name} package grammar", {stack, detail, dismissable: true}) else grammar.packageName = @name From 7a3065e0fb1fb27c8bec7e8be2efdae53d18148d Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 4 Mar 2015 11:18:10 -0800 Subject: [PATCH 28/28] Add spec for invalid settings notification --- .../package-with-invalid-settings/package.json | 4 ++++ .../settings/settings.json | 1 + spec/package-manager-spec.coffee | 13 +++++++++++++ src/package.coffee | 2 +- 4 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 spec/fixtures/packages/package-with-invalid-settings/package.json create mode 100644 spec/fixtures/packages/package-with-invalid-settings/settings/settings.json diff --git a/spec/fixtures/packages/package-with-invalid-settings/package.json b/spec/fixtures/packages/package-with-invalid-settings/package.json new file mode 100644 index 000000000..b09dccb8b --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-settings/package.json @@ -0,0 +1,4 @@ +{ + "name": "package-with-invalid-settings", + "version": "1.0.0" +} diff --git a/spec/fixtures/packages/package-with-invalid-settings/settings/settings.json b/spec/fixtures/packages/package-with-invalid-settings/settings/settings.json new file mode 100644 index 000000000..41ec4c527 --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-settings/settings/settings.json @@ -0,0 +1 @@ +>< diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index 76adfe847..6efb17a55 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -239,6 +239,19 @@ describe "PackageManager", -> expect(addErrorHandler.callCount).toBe 1 expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load a package-with-invalid-grammar package grammar") + it "adds a notification when the settings are invalid", -> + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) + + expect(-> atom.packages.activatePackage('package-with-invalid-settings')).not.toThrow() + + waitsFor -> + addErrorHandler.callCount > 0 + + runs -> + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load the package-with-invalid-settings package settings") + describe "when the package has no main module", -> it "does not throw an exception", -> spyOn(console, "error") diff --git a/src/package.coffee b/src/package.coffee index 0c4f7319d..758894952 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -324,7 +324,7 @@ class Package ScopedProperties.load settingsPath, (error, settings) => if error? detail = "#{error.message} in #{settingsPath}" - stack += "#{error.stack}\n at #{settingsPath}:1:1" + stack = "#{error.stack}\n at #{settingsPath}:1:1" atom.notifications.addFatalError("Failed to load the #{@name} package settings", {stack, detail, dismissable: true}) else @settings.push(settings)