Merge pull request #9687 from atom/mb-deprecate-load-time-package-code

Provide package.json fields so we can defer requiring packages' main modules

package.json

@@ -76,7 +76,7 @@
     "autocomplete-css": "0.11.0",
     "autocomplete-html": "0.7.2",
     "autocomplete-plus": "2.23.1",
-    "autocomplete-snippets": "1.8.0",
+    "autocomplete-snippets": "1.9.0",
     "autoflow": "0.26.0",
     "autosave": "0.23.0",
     "background-tips": "0.26.0",

spec/fixtures/packages/package-with-deserializers/deserializer-1.js

@@ -0,0 +1,6 @@
+module.exports = function (state) {
+  return {
+    wasDeserializedBy: 'Deserializer1',
+    state: state
+  }
+}

spec/fixtures/packages/package-with-deserializers/deserializer-2.js

@@ -0,0 +1,6 @@
+module.exports = function (state) {
+  return {
+    wasDeserializedBy: 'Deserializer2',
+    state: state
+  }
+}

spec/fixtures/packages/package-with-deserializers/index.js

@@ -0,0 +1,3 @@
+module.exports = {
+  activate: function() {}
+}

spec/fixtures/packages/package-with-deserializers/package.json

@@ -0,0 +1,9 @@
+{
+  "name": "package-with-deserializers",
+  "version": "1.0.0",
+  "main": "./index",
+  "deserializers": {
+    "Deserializer1": "./deserializer-1.js",
+    "Deserializer2": "./deserializer-2.js"
+  }
+}

spec/fixtures/packages/package-with-eval-time-api-calls/index.js

@@ -0,0 +1,5 @@
+atom.deserializers.add('MyDeserializer', function (state) {
+  return {state: state, a: 'b'}
+})
+
+exports.activate = function () {}

spec/fixtures/packages/package-with-eval-time-api-calls/package.json

@@ -0,0 +1,5 @@
+{
+  "name": "package-with-eval-time-api-calls",
+  "version": "1.2.3",
+  "main": "./index"
+}

spec/fixtures/packages/package-with-json-config-schema/package.json

@@ -0,0 +1,13 @@
+{
+  "name": "package-with-json-config-schema",
+  "configSchema": {
+    "a": {
+      "type": "number",
+      "default": 5
+    },
+    "b": {
+      "type": "string",
+      "default": "five"
+    }
+  }
+}

spec/fixtures/packages/package-with-main/package.cson

@@ -1 +1,2 @@
 'main': 'main-module.coffee'
+'version': '2.3.4'

spec/fixtures/packages/package-with-view-providers/deserializer.js

@@ -0,0 +1,3 @@
+module.exports = function (state) {
+  return {state: state}
+}

spec/fixtures/packages/package-with-view-providers/index.js

@@ -0,0 +1,3 @@
+module.exports = {
+  activate: function() {}
+}

spec/fixtures/packages/package-with-view-providers/package.json

@@ -0,0 +1,12 @@
+{
+  "name": "package-with-view-providers",
+  "main": "./index",
+  "version": "1.0.0",
+  "deserializers": {
+    "DeserializerFromPackageWithViewProviders": "./deserializer"
+  },
+  "viewProviders": [
+    "./view-provider-1",
+    "./view-provider-2"
+  ]
+}

spec/fixtures/packages/package-with-view-providers/view-provider-1.js

@@ -0,0 +1,9 @@
+'use strict'
+
+module.exports = function (model) {
+  if (model.worksWithViewProvider1) {
+    let element = document.createElement('div')
+    element.dataset['createdBy'] = 'view-provider-1'
+    return element
+  }
+}

spec/fixtures/packages/package-with-view-providers/view-provider-2.js

@@ -0,0 +1,9 @@
+'use strict'
+
+module.exports = function (model) {
+  if (model.worksWithViewProvider2) {
+    let element = document.createElement('div')
+    element.dataset['createdBy'] = 'view-provider-2'
+    return element
+  }
+}

spec/package-manager-spec.coffee

@@ -1,6 +1,7 @@
 path = require 'path'
 Package = require '../src/package'
 {Disposable} = require 'atom'
+{mockLocalStorage} = require './spec-helper'
 
 describe "PackageManager", ->
   workspaceElement = null
@@ -79,6 +80,111 @@ describe "PackageManager", ->
 
       expect(loadedPackage.name).toBe "package-with-main"
 
+    it "registers any deserializers specified in the package's package.json", ->
+      pack = atom.packages.loadPackage("package-with-deserializers")
+
+      state1 = {deserializer: 'Deserializer1', a: 'b'}
+      expect(atom.deserializers.deserialize(state1)).toEqual {
+        wasDeserializedBy: 'Deserializer1'
+        state: state1
+      }
+
+      state2 = {deserializer: 'Deserializer2', c: 'd'}
+      expect(atom.deserializers.deserialize(state2)).toEqual {
+        wasDeserializedBy: 'Deserializer2'
+        state: state2
+      }
+
+      expect(pack.mainModule).toBeNull()
+
+    describe "when there are view providers specified in the package's package.json", ->
+      model1 = {worksWithViewProvider1: true}
+      model2 = {worksWithViewProvider2: true}
+
+      afterEach ->
+        atom.packages.deactivatePackage('package-with-view-providers')
+        atom.packages.unloadPackage('package-with-view-providers')
+
+      it "does not load the view providers immediately", ->
+        pack = atom.packages.loadPackage("package-with-view-providers")
+        expect(pack.mainModule).toBeNull()
+
+        expect(-> atom.views.getView(model1)).toThrow()
+        expect(-> atom.views.getView(model2)).toThrow()
+
+      it "registers the view providers when the package is activated", ->
+        pack = atom.packages.loadPackage("package-with-view-providers")
+
+        waitsForPromise ->
+          atom.packages.activatePackage("package-with-view-providers").then ->
+            element1 = atom.views.getView(model1)
+            expect(element1 instanceof HTMLDivElement).toBe true
+            expect(element1.dataset.createdBy).toBe 'view-provider-1'
+
+            element2 = atom.views.getView(model2)
+            expect(element2 instanceof HTMLDivElement).toBe true
+            expect(element2.dataset.createdBy).toBe 'view-provider-2'
+
+      it "registers the view providers when any of the package's deserializers are used", ->
+        pack = atom.packages.loadPackage("package-with-view-providers")
+
+        spyOn(atom.views, 'addViewProvider').andCallThrough()
+        atom.deserializers.deserialize({
+          deserializer: 'DeserializerFromPackageWithViewProviders',
+          a: 'b'
+        })
+        expect(atom.views.addViewProvider.callCount).toBe 2
+
+        atom.deserializers.deserialize({
+          deserializer: 'DeserializerFromPackageWithViewProviders',
+          a: 'b'
+        })
+        expect(atom.views.addViewProvider.callCount).toBe 2
+
+        element1 = atom.views.getView(model1)
+        expect(element1 instanceof HTMLDivElement).toBe true
+        expect(element1.dataset.createdBy).toBe 'view-provider-1'
+
+        element2 = atom.views.getView(model2)
+        expect(element2 instanceof HTMLDivElement).toBe true
+        expect(element2.dataset.createdBy).toBe 'view-provider-2'
+
+    it "registers the config schema in the package's metadata, if present", ->
+      pack = atom.packages.loadPackage("package-with-json-config-schema")
+
+      expect(atom.config.getSchema('package-with-json-config-schema')).toEqual {
+        type: 'object'
+        properties: {
+          a: {type: 'number', default: 5}
+          b: {type: 'string', default: 'five'}
+        }
+      }
+
+      expect(pack.mainModule).toBeNull()
+
+    describe "when a package does not have deserializers, view providers or a config schema in its package.json", ->
+      beforeEach ->
+        atom.packages.unloadPackage('package-with-main')
+        mockLocalStorage()
+
+      it "defers loading the package's main module if the package previously used no Atom APIs when its main module was required", ->
+        pack1 = atom.packages.loadPackage('package-with-main')
+        expect(pack1.mainModule).toBeDefined()
+
+        atom.packages.unloadPackage('package-with-main')
+
+        pack2 = atom.packages.loadPackage('package-with-main')
+        expect(pack2.mainModule).toBeNull()
+
+      it "does not defer loading the package's main module if the package previously used Atom APIs when its main module was required", ->
+        pack1 = atom.packages.loadPackage('package-with-eval-time-api-calls')
+        expect(pack1.mainModule).toBeDefined()
+
+        atom.packages.unloadPackage('package-with-eval-time-api-calls')
+
+        pack2 = atom.packages.loadPackage('package-with-eval-time-api-calls')
+        expect(pack2.mainModule).not.toBeNull()
+
   describe "::unloadPackage(name)", ->
     describe "when the package is active", ->
       it "throws an error", ->

spec/package-spec.coffee

@@ -1,6 +1,7 @@
 path = require 'path'
 Package = require '../src/package'
 ThemePackage = require '../src/theme-package'
+{mockLocalStorage} = require './spec-helper'
 
 describe "Package", ->
   build = (constructor, path) ->
@@ -10,6 +11,7 @@ describe "Package", ->
       keymapManager: atom.keymaps, commandRegistry: atom.command,
       grammarRegistry: atom.grammars, themeManager: atom.themes,
       menuManager: atom.menu, contextMenuManager: atom.contextMenu,
+      deserializerManager: atom.deserializers, viewRegistry: atom.views,
       devMode: false
     )
 
@@ -19,10 +21,7 @@ describe "Package", ->
 
   describe "when the package contains incompatible native modules", ->
     beforeEach ->
-      items = {}
-      spyOn(global.localStorage, 'setItem').andCallFake (key, item) -> items[key] = item; undefined
-      spyOn(global.localStorage, 'getItem').andCallFake (key) -> items[key] ? null
-      spyOn(global.localStorage, 'removeItem').andCallFake (key) -> delete items[key]; undefined
+      mockLocalStorage()
 
     it "does not activate it", ->
       packagePath = atom.project.getDirectories()[0]?.resolve('packages/package-with-incompatible-native-module')
@@ -54,10 +53,7 @@ describe "Package", ->
 
   describe "::rebuild()", ->
     beforeEach ->
-      items = {}
-      spyOn(global.localStorage, 'setItem').andCallFake (key, item) -> items[key] = item; undefined
-      spyOn(global.localStorage, 'getItem').andCallFake (key) -> items[key] ? null
-      spyOn(global.localStorage, 'removeItem').andCallFake (key) -> delete items[key]; undefined
+      mockLocalStorage()
 
     it "returns a promise resolving to the results of `apm rebuild`", ->
       packagePath = atom.project.getDirectories()[0]?.resolve('packages/package-with-index')

spec/spec-helper.coffee

@@ -265,3 +265,9 @@ window.advanceClock = (delta=1) ->
       true
 
   callback() for callback in callbacks
+
+exports.mockLocalStorage = ->
+  items = {}
+  spyOn(global.localStorage, 'setItem').andCallFake (key, item) -> items[key] = item.toString(); undefined
+  spyOn(global.localStorage, 'getItem').andCallFake (key) -> items[key] ? null
+  spyOn(global.localStorage, 'removeItem').andCallFake (key) -> delete items[key]; undefined

spec/view-registry-spec.coffee

@@ -47,6 +47,21 @@ describe "ViewRegistry", ->
           expect(view2 instanceof TestView).toBe true
           expect(view2.model).toBe subclassModel
 
+      describe "when a view provider is registered generically, and works with the object", ->
+        it "constructs a view element and assigns the model on it", ->
+          model = {a: 'b'}
+
+          registry.addViewProvider (model) ->
+            if model.a is 'b'
+              element = document.createElement('div')
+              element.className = 'test-element'
+              element
+
+          view = registry.getView({a: 'b'})
+          expect(view.className).toBe 'test-element'
+
+          expect(-> registry.getView({a: 'c'})).toThrow()
+
       describe "when no view provider is registered for the object's constructor", ->
         it "throws an exception", ->
           expect(-> registry.getView(new Object)).toThrow()

src/atom-environment.coffee

@@ -151,7 +151,7 @@ class AtomEnvironment extends Model
     @packages = new PackageManager({
       devMode, configDirPath, resourcePath, safeMode, @config, styleManager: @styles,
       commandRegistry: @commands, keymapManager: @keymaps, notificationManager: @notifications,
-      grammarRegistry: @grammars
+      grammarRegistry: @grammars, deserializerManager: @deserializers, viewRegistry: @views
     })
 
     @themes = new ThemeManager({

src/deserializer-manager.coffee

@@ -39,6 +39,9 @@ class DeserializerManager
       delete @deserializers[deserializer.name] for deserializer in deserializers
       return
 
+  getDeserializerCount: ->
+    Object.keys(@deserializers).length
+
   # Public: Deserialize the state and params.
   #
   # * `state` The state {Object} to deserialize.

src/package-manager.coffee

@@ -31,7 +31,8 @@ class PackageManager
   constructor: (params) ->
     {
       configDirPath, @devMode, safeMode, @resourcePath, @config, @styleManager,
-      @notificationManager, @keymapManager, @commandRegistry, @grammarRegistry
+      @notificationManager, @keymapManager, @commandRegistry, @grammarRegistry,
+      @deserializerManager, @viewRegistry
     } = params
 
     @emitter = new Emitter
@@ -375,7 +376,8 @@ class PackageManager
       options = {
         path: packagePath, metadata, packageManager: this, @config, @styleManager,
         @commandRegistry, @keymapManager, @devMode, @notificationManager,
-        @grammarRegistry, @themeManager, @menuManager, @contextMenuManager
+        @grammarRegistry, @themeManager, @menuManager, @contextMenuManager,
+        @deserializerManager, @viewRegistry
       }
       if metadata.theme
         pack = new ThemePackage(options)

src/package.coffee

@@ -33,7 +33,7 @@ class Package
     {
       @path, @metadata, @packageManager, @config, @styleManager, @commandRegistry,
       @keymapManager, @devMode, @notificationManager, @grammarRegistry, @themeManager,
-      @menuManager, @contextMenuManager
+      @menuManager, @contextMenuManager, @deserializerManager, @viewRegistry
     } = params
 
     @emitter = new Emitter
@@ -84,12 +84,24 @@ class Package
         @loadKeymaps()
         @loadMenus()
         @loadStylesheets()
+        @loadDeserializers()
+        @configSchemaRegisteredOnLoad = @registerConfigSchemaFromMetadata()
         @settingsPromise = @loadSettings()
-        @requireMainModule() unless @mainModule? or @activationShouldBeDeferred()
+        if @shouldRequireMainModuleOnLoad() and not @mainModule?
+          @requireMainModule()
       catch error
         @handleError("Failed to load the #{@name} package", error)
     this
 
+  shouldRequireMainModuleOnLoad: ->
+    not (
+      @metadata.deserializers? or
+      @metadata.viewProviders? or
+      @metadata.configSchema? or
+      @activationShouldBeDeferred() or
+      localStorage.getItem(@getCanDeferMainModuleRequireStorageKey()) is 'true'
+    )
+
   reset: ->
     @stylesheets = []
     @keymaps = []
@@ -117,9 +129,12 @@ class Package
 
   activateNow: ->
     try
-      @activateConfig()
+      @requireMainModule() unless @mainModule?
+      @configSchemaRegisteredOnActivate = @registerConfigSchemaFromMainModule()
+      @registerViewProviders()
       @activateStylesheets()
       if @mainModule? and not @mainActivated
+        @mainModule.activateConfig?()
         @mainModule.activate?(@packageManager.getPackageState(@name) ? {})
         @mainActivated = true
         @activateServices()
@@ -128,15 +143,22 @@ class Package
 
     @resolveActivationPromise?()
 
-  activateConfig: ->
-    return if @configActivated
+  registerConfigSchemaFromMetadata: ->
+    if configSchema = @metadata.configSchema
+      @config.setSchema @name, {type: 'object', properties: configSchema}
+      true
+    else
+      false
 
-    @requireMainModule() unless @mainModule?
-    if @mainModule?
+  registerConfigSchemaFromMainModule: ->
+    if @mainModule? and not @configSchemaRegisteredOnLoad
       if @mainModule.config? and typeof @mainModule.config is 'object'
         @config.setSchema @name, {type: 'object', properties: @mainModule.config}
-      @mainModule.activateConfig?()
-    @configActivated = true
+        return true
+    false
+
+  # TODO: Remove. Settings view calls this method currently.
+  activateConfig: -> @registerConfigSchemaFromMainModule()
 
   activateStylesheets: ->
     return if @stylesheetsActivated
@@ -253,6 +275,26 @@ class Package
     @stylesheets = @getStylesheetPaths().map (stylesheetPath) =>
       [stylesheetPath, @themeManager.loadStylesheet(stylesheetPath, true)]
 
+  loadDeserializers: ->
+    if @metadata.deserializers?
+      for name, implementationPath of @metadata.deserializers
+        do =>
+          deserializePath = path.join(@path, implementationPath)
+          deserializeFunction = null
+          atom.deserializers.add
+            name: name,
+            deserialize: =>
+              @registerViewProviders()
+              deserializeFunction ?= require(deserializePath)
+              deserializeFunction.apply(this, arguments)
+      return
+
+  registerViewProviders: ->
+    if @metadata.viewProviders? and not @registeredViewProviders
+      for implementationPath in @metadata.viewProviders
+        @viewRegistry.addViewProvider(require(path.join(@path, implementationPath)))
+      @registeredViewProviders = true
+
   getStylesheetsPath: ->
     path.join(@path, 'styles')
 
@@ -343,21 +385,18 @@ class Package
     @activationPromise = null
     @resolveActivationPromise = null
     @activationCommandSubscriptions?.dispose()
+    @configSchemaRegisteredOnActivate = false
     @deactivateResources()
-    @deactivateConfig()
     @deactivateKeymaps()
     if @mainActivated
       try
         @mainModule?.deactivate?()
+        @mainModule?.deactivateConfig?()
         @mainActivated = false
       catch e
         console.error "Error deactivating package '#{@name}'", e.stack
     @emitter.emit 'did-deactivate'
 
-  deactivateConfig: ->
-    @mainModule?.deactivateConfig?()
-    @configActivated = false
-
   deactivateResources: ->
     grammar.deactivate() for grammar in @grammars
     settings.deactivate() for settings in @settings
@@ -392,7 +431,13 @@ class Package
     mainModulePath = @getMainModulePath()
     if fs.isFileSync(mainModulePath)
       @mainModuleRequired = true
+
+      previousViewProviderCount = @viewRegistry.getViewProviderCount()
+      previousDeserializerCount = @deserializerManager.getDeserializerCount()
       @mainModule = require(mainModulePath)
+      if (@viewRegistry.getViewProviderCount() is previousViewProviderCount and
+          @deserializerManager.getDeserializerCount() is previousDeserializerCount)
+        localStorage.setItem(@getCanDeferMainModuleRequireStorageKey(), 'true')
 
   getMainModulePath: ->
     return @mainModulePath if @resolvedMainModulePath
@@ -586,6 +631,9 @@ class Package
     electronVersion = process.versions['electron'] ? process.versions['atom-shell']
     "installed-packages:#{@name}:#{@metadata.version}:electron-#{electronVersion}:incompatible-native-modules"
 
+  getCanDeferMainModuleRequireStorageKey: ->
+    "installed-packages:#{@name}:#{@metadata.version}:can-defer-main-module-require"
+
   # Get the incompatible native modules that this package depends on.
   # This recurses through all dependencies and requires all modules that
   # contain a `.node` file.

src/view-registry.coffee

@@ -3,6 +3,8 @@ Grim = require 'grim'
 {Disposable} = require 'event-kit'
 _ = require 'underscore-plus'
 
+AnyConstructor = Symbol('any-constructor')
+
 # Essential: `ViewRegistry` handles the association between model and view
 # types in Atom. We call this association a View Provider. As in, for a given
 # model, this class can provide a view via {::getView}, as long as the
@@ -76,16 +78,27 @@ class ViewRegistry
   #   textEditorElement
   # ```
   #
-  # * `modelConstructor` Constructor {Function} for your model.
+  # * `modelConstructor` (optional) Constructor {Function} for your model. If
+  #   a constructor is given, the `createView` function will only be used
+  #   for model objects inheriting from that constructor. Otherwise, it will
+  #   will be called for any object.
   # * `createView` Factory {Function} that is passed an instance of your model
-  #   and must return a subclass of `HTMLElement` or `undefined`.
+  #   and must return a subclass of `HTMLElement` or `undefined`. If it returns
+  #   `undefined`, then the registry will continue to search for other view
+  #   providers.
   #
   # Returns a {Disposable} on which `.dispose()` can be called to remove the
   # added provider.
   addViewProvider: (modelConstructor, createView) ->
     if arguments.length is 1
-      Grim.deprecate("atom.views.addViewProvider now takes 2 arguments: a model constructor and a createView function. See docs for details.")
-      provider = modelConstructor
+      switch typeof modelConstructor
+        when 'function'
+          provider = {createView: modelConstructor, modelConstructor: AnyConstructor}
+        when 'object'
+          Grim.deprecate("atom.views.addViewProvider now takes 2 arguments: a model constructor and a createView function. See docs for details.")
+          provider = modelConstructor
+        else
+          throw new TypeError("Arguments to addViewProvider must be functions")
     else
       provider = {modelConstructor, createView}
 
@@ -93,6 +106,9 @@ class ViewRegistry
     new Disposable =>
       @providers = @providers.filter (p) -> p isnt provider
 
+  getViewProviderCount: ->
+    @providers.length
+
   # Essential: Get the view associated with an object in the workspace.
   #
   # If you're just *using* the workspace, you shouldn't need to access the view
@@ -153,25 +169,34 @@ class ViewRegistry
 
   createView: (object) ->
     if object instanceof HTMLElement
-      object
-    else if object?.element instanceof HTMLElement
-      object.element
-    else if object?.jquery
-      object[0]
-    else if provider = @findProvider(object)
-      element = provider.createView?(object, @atomEnvironment)
-      unless element?
-        element = new provider.viewConstructor
-        element.initialize?(object) ? element.setModel?(object)
-      element
-    else if viewConstructor = object?.getViewClass?()
-      view = new viewConstructor(object)
-      view[0]
-    else
-      throw new Error("Can't create a view for #{object.constructor.name} instance. Please register a view provider.")
+      return object
 
-  findProvider: (object) ->
-    find @providers, ({modelConstructor}) -> object instanceof modelConstructor
+    if object?.element instanceof HTMLElement
+      return object.element
+
+    if object?.jquery
+      return object[0]
+
+    for provider in @providers
+      if provider.modelConstructor is AnyConstructor
+        if element = provider.createView(object, @atomEnvironment)
+          return element
+        continue
+
+      if object instanceof provider.modelConstructor
+        if element = provider.createView?(object, @atomEnvironment)
+          return element
+
+        if viewConstructor = provider.viewConstructor
+          element = new viewConstructor
+          element.initialize?(object) ? element.setModel?(object)
+          return element
+
+    if viewConstructor = object?.getViewClass?()
+      view = new viewConstructor(object)
+      return view[0]
+
+    throw new Error("Can't create a view for #{object.constructor.name} instance. Please register a view provider.")
 
   updateDocument: (fn) ->
     @documentWriters.push(fn)