Merge pull request #14168 from atom/as-fix-module-cache-for-preloaded-packages

Fix adding bundled packages to `ModuleCache` and increase test coverage
2026-04-06 03:02:13 -04:00 · 2017-04-11 10:12:54 +02:00
parent b2c33f0184 d564bfc983
commit 74ff9fdb91
3 changed files with 220 additions and 68 deletions
--- a/spec/package-manager-spec.coffee
+++ b/spec/package-manager-spec.coffee
@@ -5,6 +5,7 @@ fs = require 'fs-plus'
 {Disposable} = require 'atom'
 {buildKeydownEvent} = require '../src/keymap-extensions'
 {mockLocalStorage} = require './spec-helper'
+ModuleCache = require '../src/module-cache'

 describe "PackageManager", ->
  createTestElement = (className) ->
@@ -12,6 +13,9 @@ describe "PackageManager", ->
    element.className = className
    element

+  beforeEach ->
+    spyOn(ModuleCache, 'add')
+
  afterEach ->
    temp.cleanupSync()

@@ -243,6 +247,148 @@ describe "PackageManager", ->
        pack2 = atom.packages.loadPackage('package-with-eval-time-api-calls')
        expect(pack2.mainModule).not.toBeNull()

+  describe "::loadAvailablePackage(availablePackage)", ->
+    describe "if the package was preloaded", ->
+      it "adds the package path to the module cache", ->
+        availablePackage = atom.packages.getAvailablePackages().find (p) -> p.name is 'spell-check'
+        availablePackage.isBundled = true
+        expect(atom.packages.preloadedPackages[availablePackage.name]).toBeUndefined()
+        expect(atom.packages.isPackageLoaded(availablePackage.name)).toBe(false)
+
+        metadata = atom.packages.loadPackageMetadata(availablePackage)
+        atom.packages.preloadPackage(
+          availablePackage.name,
+          {
+            rootDirPath: path.relative(atom.packages.resourcePath, availablePackage.path),
+            metadata
+          }
+        )
+        atom.packages.loadAvailablePackage(availablePackage)
+        expect(atom.packages.isPackageLoaded(availablePackage.name)).toBe(true)
+        expect(ModuleCache.add).toHaveBeenCalledWith(availablePackage.path, metadata)
+
+      it "deactivates it if it had been disabled", ->
+        availablePackage = atom.packages.getAvailablePackages().find (p) -> p.name is 'spell-check'
+        availablePackage.isBundled = true
+        expect(atom.packages.preloadedPackages[availablePackage.name]).toBeUndefined()
+        expect(atom.packages.isPackageLoaded(availablePackage.name)).toBe(false)
+
+        metadata = atom.packages.loadPackageMetadata(availablePackage)
+        preloadedPackage = atom.packages.preloadPackage(
+          availablePackage.name,
+          {
+            rootDirPath: path.relative(atom.packages.resourcePath, availablePackage.path),
+            metadata
+          }
+        )
+        expect(preloadedPackage.keymapActivated).toBe(true)
+        expect(preloadedPackage.settingsActivated).toBe(true)
+        expect(preloadedPackage.menusActivated).toBe(true)
+
+        atom.packages.loadAvailablePackage(availablePackage, new Set([availablePackage.name]))
+        expect(atom.packages.isPackageLoaded(availablePackage.name)).toBe(false)
+        expect(preloadedPackage.keymapActivated).toBe(false)
+        expect(preloadedPackage.settingsActivated).toBe(false)
+        expect(preloadedPackage.menusActivated).toBe(false)
+
+      it "deactivates it and reloads the new one if trying to load the same package outside of the bundle", ->
+        availablePackage = atom.packages.getAvailablePackages().find (p) -> p.name is 'spell-check'
+        availablePackage.isBundled = true
+        expect(atom.packages.preloadedPackages[availablePackage.name]).toBeUndefined()
+        expect(atom.packages.isPackageLoaded(availablePackage.name)).toBe(false)
+
+        metadata = atom.packages.loadPackageMetadata(availablePackage)
+        preloadedPackage = atom.packages.preloadPackage(
+          availablePackage.name,
+          {
+            rootDirPath: path.relative(atom.packages.resourcePath, availablePackage.path),
+            metadata
+          }
+        )
+        expect(preloadedPackage.keymapActivated).toBe(true)
+        expect(preloadedPackage.settingsActivated).toBe(true)
+        expect(preloadedPackage.menusActivated).toBe(true)
+
+        availablePackage.isBundled = false
+        atom.packages.loadAvailablePackage(availablePackage)
+        expect(atom.packages.isPackageLoaded(availablePackage.name)).toBe(true)
+        expect(preloadedPackage.keymapActivated).toBe(false)
+        expect(preloadedPackage.settingsActivated).toBe(false)
+        expect(preloadedPackage.menusActivated).toBe(false)
+
+    describe "if the package was not preloaded", ->
+      it "adds the package path to the module cache", ->
+        availablePackage = atom.packages.getAvailablePackages().find (p) -> p.name is 'spell-check'
+        availablePackage.isBundled = true
+        metadata = atom.packages.loadPackageMetadata(availablePackage)
+        atom.packages.loadAvailablePackage(availablePackage)
+        expect(ModuleCache.add).toHaveBeenCalledWith(availablePackage.path, metadata)
+
+  describe "preloading", ->
+    it "requires the main module, loads the config schema and activates keymaps, menus and settings without reactivating them during package activation", ->
+      availablePackage = atom.packages.getAvailablePackages().find (p) -> p.name is 'spell-check'
+      availablePackage.isBundled = true
+      expect(atom.packages.preloadedPackages[availablePackage.name]).toBeUndefined()
+      expect(atom.packages.isPackageLoaded(availablePackage.name)).toBe(false)
+
+      metadata = atom.packages.loadPackageMetadata(availablePackage)
+      preloadedPackage = atom.packages.preloadPackage(
+        availablePackage.name,
+        {
+          rootDirPath: path.relative(atom.packages.resourcePath, availablePackage.path),
+          metadata
+        }
+      )
+      expect(preloadedPackage.keymapActivated).toBe(true)
+      expect(preloadedPackage.settingsActivated).toBe(true)
+      expect(preloadedPackage.menusActivated).toBe(true)
+      expect(preloadedPackage.mainModule).toBeTruthy()
+      expect(preloadedPackage.configSchemaRegisteredOnLoad).toBeTruthy()
+
+      atom.packages.loadAvailablePackage(availablePackage)
+
+      spyOn(atom.keymaps, 'add')
+      spyOn(atom.menu, 'add')
+      spyOn(atom.contextMenu, 'add')
+      spyOn(atom.config, 'setSchema')
+      atom.packages.activatePackage(availablePackage.name)
+
+      expect(atom.keymaps.add).not.toHaveBeenCalled()
+      expect(atom.menu.add).not.toHaveBeenCalled()
+      expect(atom.contextMenu.add).not.toHaveBeenCalled()
+      expect(atom.config.setSchema).not.toHaveBeenCalled()
+      expect(preloadedPackage.keymapActivated).toBe(true)
+      expect(preloadedPackage.settingsActivated).toBe(true)
+      expect(preloadedPackage.menusActivated).toBe(true)
+      expect(preloadedPackage.mainModule).toBeTruthy()
+      expect(preloadedPackage.configSchemaRegisteredOnLoad).toBeTruthy()
+
+    it "deactivates disabled keymaps during package activation", ->
+      availablePackage = atom.packages.getAvailablePackages().find (p) -> p.name is 'spell-check'
+      availablePackage.isBundled = true
+      expect(atom.packages.preloadedPackages[availablePackage.name]).toBeUndefined()
+      expect(atom.packages.isPackageLoaded(availablePackage.name)).toBe(false)
+
+      metadata = atom.packages.loadPackageMetadata(availablePackage)
+      preloadedPackage = atom.packages.preloadPackage(
+        availablePackage.name,
+        {
+          rootDirPath: path.relative(atom.packages.resourcePath, availablePackage.path),
+          metadata
+        }
+      )
+      expect(preloadedPackage.keymapActivated).toBe(true)
+      expect(preloadedPackage.settingsActivated).toBe(true)
+      expect(preloadedPackage.menusActivated).toBe(true)
+
+      atom.packages.loadAvailablePackage(availablePackage)
+      atom.config.set("core.packagesWithKeymapsDisabled", [availablePackage.name])
+      atom.packages.activatePackage(availablePackage.name)
+
+      expect(preloadedPackage.keymapActivated).toBe(false)
+      expect(preloadedPackage.settingsActivated).toBe(true)
+      expect(preloadedPackage.menusActivated).toBe(true)
+
  describe "::unloadPackage(name)", ->
    describe "when the package is active", ->
      it "throws an error", ->
--- a/src/package-manager.coffee
+++ b/src/package-manager.coffee
@@ -71,6 +71,7 @@ class PackageManager
    @serviceHub.clear()
    @deactivatePackages()
    @loadedPackages = {}
+    @preloadedPackages = {}
    @packageStates = {}
    @triggeredActivationHooks.clear()

@@ -380,27 +381,30 @@ class PackageManager

  preloadPackages: ->
    for packageName, pack of @packagesCache
-      metadata = pack.metadata ? {}
-      unless typeof metadata.name is 'string' and metadata.name.length > 0
-        metadata.name = packageName
+      @preloadPackage(packageName, pack)

-      if metadata.repository?.type is 'git' and typeof metadata.repository.url is 'string'
-        metadata.repository.url = metadata.repository.url.replace(/(^git\+)|(\.git$)/g, '')
+  preloadPackage: (packageName, pack) ->
+    metadata = pack.metadata ? {}
+    unless typeof metadata.name is 'string' and metadata.name.length > 0
+      metadata.name = packageName

-      options = {
-        path: pack.rootDirPath, name: packageName, preloadedPackage: true,
-        bundledPackage: true, metadata, packageManager: this, @config,
-        @styleManager, @commandRegistry, @keymapManager,
-        @notificationManager, @grammarRegistry, @themeManager, @menuManager,
-        @contextMenuManager, @deserializerManager, @viewRegistry
-      }
-      if metadata.theme
-        pack = new ThemePackage(options)
-      else
-        pack = new Package(options)
+    if metadata.repository?.type is 'git' and typeof metadata.repository.url is 'string'
+      metadata.repository.url = metadata.repository.url.replace(/(^git\+)|(\.git$)/g, '')

-      pack.preload()
-      @preloadedPackages[packageName] = pack
+    options = {
+      path: pack.rootDirPath, name: packageName, preloadedPackage: true,
+      bundledPackage: true, metadata, packageManager: this, @config,
+      @styleManager, @commandRegistry, @keymapManager,
+      @notificationManager, @grammarRegistry, @themeManager, @menuManager,
+      @contextMenuManager, @deserializerManager, @viewRegistry
+    }
+    if metadata.theme
+      pack = new ThemePackage(options)
+    else
+      pack = new Package(options)
+
+    pack.preload()
+    @preloadedPackages[packageName] = pack

  loadPackages: ->
    # Ensure atom exports is already in the require cache so the load time
@@ -410,12 +414,7 @@ class PackageManager
    disabledPackageNames = new Set(@config.get('core.disabledPackages'))
    @config.transact =>
      for pack in @getAvailablePackages()
-        if disabledPackageNames.has(pack.name)
-          if preloadedPackage = @preloadedPackages[pack.name]
-            preloadedPackage.deactivate()
-            delete preloadedPackage[pack.name]
-        else
-          @loadAvailablePackage(pack)
+        @loadAvailablePackage(pack, disabledPackageNames)
      return
    @initialPackagesLoaded = true
    @emitter.emit 'did-load-initial-packages'
@@ -432,47 +431,53 @@ class PackageManager
      console.warn "Could not resolve '#{nameOrPath}' to a package path"
      null

-  loadAvailablePackage: (availablePackage) ->
-    loadedPackage = @getLoadedPackage(availablePackage.name)
-    if loadedPackage?
-      loadedPackage
-    else
-      preloadedPackage = @preloadedPackages[availablePackage.name]
+  loadAvailablePackage: (availablePackage, disabledPackageNames) ->
+    preloadedPackage = @preloadedPackages[availablePackage.name]
+
+    if disabledPackageNames?.has(availablePackage.name)
      if preloadedPackage?
-        if availablePackage.isBundled
-          preloadedPackage.finishLoading()
-          @loadedPackages[availablePackage.name] = preloadedPackage
-          return preloadedPackage
-        else
-          preloadedPackage.deactivate()
-          delete preloadedPackage[availablePackage.name]
+        preloadedPackage.deactivate()
+        delete preloadedPackage[availablePackage.name]
+    else
+      loadedPackage = @getLoadedPackage(availablePackage.name)
+      if loadedPackage?
+        loadedPackage
+      else
+        if preloadedPackage?
+          if availablePackage.isBundled
+            preloadedPackage.finishLoading()
+            @loadedPackages[availablePackage.name] = preloadedPackage
+            return preloadedPackage
+          else
+            preloadedPackage.deactivate()
+            delete preloadedPackage[availablePackage.name]

-      try
-        metadata = @loadPackageMetadata(availablePackage) ? {}
-      catch error
-        @handleMetadataError(error, availablePackage.path)
-        return null
-
-      unless availablePackage.isBundled
-        if @isDeprecatedPackage(metadata.name, metadata.version)
-          console.warn "Could not load #{metadata.name}@#{metadata.version} because it uses deprecated APIs that have been removed."
+        try
+          metadata = @loadPackageMetadata(availablePackage) ? {}
+        catch error
+          @handleMetadataError(error, availablePackage.path)
          return null

-      options = {
-        path: availablePackage.path, name: availablePackage.name, metadata,
-        bundledPackage: availablePackage.isBundled, packageManager: this,
-        @config, @styleManager, @commandRegistry, @keymapManager,
-        @notificationManager, @grammarRegistry, @themeManager, @menuManager,
-        @contextMenuManager, @deserializerManager, @viewRegistry
-      }
-      if metadata.theme
-        pack = new ThemePackage(options)
-      else
-        pack = new Package(options)
-      pack.load()
-      @loadedPackages[pack.name] = pack
-      @emitter.emit 'did-load-package', pack
-      pack
+        unless availablePackage.isBundled
+          if @isDeprecatedPackage(metadata.name, metadata.version)
+            console.warn "Could not load #{metadata.name}@#{metadata.version} because it uses deprecated APIs that have been removed."
+            return null
+
+        options = {
+          path: availablePackage.path, name: availablePackage.name, metadata,
+          bundledPackage: availablePackage.isBundled, packageManager: this,
+          @config, @styleManager, @commandRegistry, @keymapManager,
+          @notificationManager, @grammarRegistry, @themeManager, @menuManager,
+          @contextMenuManager, @deserializerManager, @viewRegistry
+        }
+        if metadata.theme
+          pack = new ThemePackage(options)
+        else
+          pack = new Package(options)
+        pack.load()
+        @loadedPackages[pack.name] = pack
+        @emitter.emit 'did-load-package', pack
+        pack

  unloadPackages: ->
    @unloadPackage(name) for name in _.keys(@loadedPackages)
--- a/src/package.coffee
+++ b/src/package.coffee
@@ -42,8 +42,6 @@ class Package
    @metadata ?= @packageManager.loadPackageMetadata(@path)
    @bundledPackage ?= @packageManager.isBundledPackagePath(@path)
    @name = @metadata?.name ? params.name ? path.basename(@path)
-    unless @bundledPackage
-      ModuleCache.add(@path, @metadata)
    @reset()

  ###
@@ -99,8 +97,9 @@ class Package
  finishLoading: ->
    @measure 'loadTime', =>
      @path = path.join(@packageManager.resourcePath, @path)
-      @loadStylesheets()
+      ModuleCache.add(@path, @metadata)

+      @loadStylesheets()
      # Unfortunately some packages are accessing `@mainModulePath`, so we need
      # to compute that variable eagerly also for preloaded packages.
      @getMainModulePath()
@@ -108,6 +107,8 @@ class Package
  load: ->
    @measure 'loadTime', =>
      try
+        ModuleCache.add(@path, @metadata)
+
        @loadKeymaps()
        @loadMenus()
        @loadStylesheets()
@@ -409,13 +410,13 @@ class Package
  loadGrammarsSync: ->
    return if @grammarsLoaded

-    if @preloadedPackage
+    if @preloadedPackage and @packageManager.packagesCache[@name]?
      grammarPaths = @packageManager.packagesCache[@name].grammarPaths
    else
      grammarPaths = fs.listSync(path.join(@path, 'grammars'), ['json', 'cson'])

    for grammarPath in grammarPaths
-      if @preloadedPackage
+      if @preloadedPackage and @packageManager.packagesCache[@name]?
        grammarPath = path.resolve(@packageManager.resourcePath, grammarPath)

      try
@@ -450,7 +451,7 @@ class Package
        callback()

    new Promise (resolve) =>
-      if @preloadedPackage
+      if @preloadedPackage and @packageManager.packagesCache[@name]?
        grammarPaths = @packageManager.packagesCache[@name].grammarPaths
        async.each grammarPaths, loadGrammar, -> resolve()
      else
@@ -476,7 +477,7 @@ class Package
        callback()

    new Promise (resolve) =>
-      if @preloadedPackage
+      if @preloadedPackage and @packageManager.packagesCache[@name]?
        for settingsPath, scopedProperties of @packageManager.packagesCache[@name].settings
          settings = new ScopedProperties("core:#{settingsPath}", scopedProperties ? {}, @config)
          @settings.push(settings)