various fixes

packages/commands/dlx/dlx.cr

@@ -1,7 +1,10 @@
+require "log"
 require "shared/constants"
 require "./config"
 
 module Commands::Dlx
+  Log = ::Log.for("zap.commands.dlx")
+
   def self.run(
     config : Core::Config,
     dlx_config : Dlx::Config

packages/commands/install/install.cr

@@ -1,4 +1,5 @@
 require "benchmark"
+require "log"
 require "concurrency/pipeline"
 require "reporter/reporter"
 require "reporter/null"
@@ -15,6 +16,8 @@ require "./linker/isolated"
 require "./linker/pnp"
 
 module Commands::Install
+  Log = ::Log.for("zap.commands.install")
+
   alias Pipeline = Concurrency::Pipeline
 
   def self.run(
@@ -65,10 +68,10 @@ module Commands::Install
       config = self.strategy_check(config, install_config, lockfile, inferred_context, reporter)
 
       # Force hoisting if the hoisting options have changed
-      self.hoisting_check(install_config, lockfile, inferred_context, reporter)
+      install_config = self.hoisting_check(install_config, lockfile, inferred_context, reporter)
 
       # Force metadata retrieval if the package extensions options have changed
-      self.package_extensions_check(install_config, lockfile, inferred_context, reporter)
+      install_config = self.package_extensions_check(install_config, lockfile, inferred_context, reporter)
 
       # Init state struct
       state = State.new(
@@ -119,6 +122,7 @@ module Commands::Install
       # Do not edit lockfile or package.json files in global mode or if the save flag is false
       unless state.config.global || !state.install_config.save
         # Write lockfile
+        Log.debug { "• Writing the lockfile" }
         state.lockfile.write(format: config.lockfile_format)
 
         # Edit and write the package.json files if the flags have been set in the config
@@ -236,7 +240,7 @@ module Commands::Install
     config
   end
 
-  private def self.hoisting_check(install_config : Install::Config, lockfile : Data::Lockfile, inferred_context : Core::Config::InferredContext, reporter : Reporter)
+  private def self.hoisting_check(install_config : Install::Config, lockfile : Data::Lockfile, inferred_context : Core::Config::InferredContext, reporter : Reporter) : Install::Config
     if lockfile.update_hoisting_shasum(inferred_context.main_package)
       if install_config.frozen_lockfile
         # If the lockfile is frozen, raise an error
@@ -246,12 +250,13 @@ module Commands::Install
       if lockfile.read_status.from_disk?
         Log.debug { "Detected a change in hoisting options in the package.json file" }
         reporter.info("Hoisting options were modified. The packages will be re-installed.")
-        install_config = install_config.copy_with(refresh_install: true)
+        return install_config.copy_with(refresh_install: true)
       end
     end
+    install_config
   end
 
-  private def self.package_extensions_check(install_config : Install::Config, lockfile : Data::Lockfile, inferred_context : Core::Config::InferredContext, reporter : Reporter)
+  private def self.package_extensions_check(install_config : Install::Config, lockfile : Data::Lockfile, inferred_context : Core::Config::InferredContext, reporter : Reporter) : Install::Config
     if lockfile.update_package_extensions_shasum(inferred_context.main_package)
       if install_config.frozen_lockfile
         # If the lockfile is frozen, raise an error
@@ -261,9 +266,10 @@ module Commands::Install
       if lockfile.read_status.from_disk?
         Log.debug { "Detected a change in package extensions options in the package.json file" }
         reporter.info("Package extensions have been modified. Package metadata will forcefully be fetched from the registry and packages will be re-installed.")
-        install_config = install_config.copy_with(force_metadata_retrieval: true, refresh_install: true)
+        return install_config.copy_with(force_metadata_retrieval: true, refresh_install: true)
       end
     end
+    install_config
   end
 
   private def self.remove_packages(state : State)

packages/commands/install/linker/classic/classic.cr

@@ -1,7 +1,10 @@
+require "log"
 require "utils/macros"
 require "../linker"
 
 class Commands::Install::Linker::Classic < Commands::Install::Linker::Base
+  Log = ::Log.for("zap.commands.install.linker.classic")
+
   record DependencyItem,
     # the dependency to install
     dependency : Data::Package,

packages/commands/install/linker/isolated/isolated.cr

@@ -1,3 +1,4 @@
+require "log"
 require "semver"
 require "shared/constants"
 require "utils/directories"
@@ -15,6 +16,8 @@ class Commands::Install::Linker::Isolated < Commands::Install::Linker::Base
   @public_hoist_patterns : Array(Regex)
   @installed_packages : Set(String) = Set(String).new
 
+  Log = ::Log.for("zap.commands.install.linker.isolated")
+
   def initialize(
     state,
     *,
@@ -103,7 +106,6 @@ class Commands::Install::Linker::Isolated < Commands::Install::Linker::Base
       end
 
       # If the package folder exists, we assume that the package dependencies were already installed too
-      package_path = install_path / package.name
       if File.directory?(install_path)
         # If there is no need to perform a full pass, we can just return the package path and skip the dependencies
         unless state.install_config.refresh_install

packages/commands/install/linker/linker.cr

@@ -3,7 +3,7 @@ require "data/package"
 require "../state"
 
 module Commands::Install::Linker
-  Log = ::Log.for("zap.linker")
+  Log = ::Log.for("zap.commands.install.linker")
 
   abstract class Base
     getter state : Commands::Install::State

packages/commands/install/linker/pnp/pnp.cr

@@ -1,3 +1,4 @@
+require "log"
 require "utils/macros"
 require "../linker"
 
@@ -9,6 +10,8 @@ class Commands::Install::Linker::PnP < Commands::Install::Linker::Base
   @relative_modules_store : Path
   @manifest : Manifest = Manifest.new
 
+  Log = ::Log.for("zap.commands.install.linker.pnp")
+
   def initialize(state : Commands::Install::State)
     super(state)
     @node_modules = Path.new(state.config.node_modules)

packages/commands/install/protocol/alias/alias.cr

@@ -1,10 +1,13 @@
+require "log"
 require "utils/misc"
 require "extensions/object"
 require "../../resolver"
 require "../base"
 require "../registry"
 
 struct Commands::Install::Protocol::Alias < Commands::Install::Protocol::Base
+  Log = ::Log.for("zap.commands.install.protocol.alias")
+
   def self.normalize?(str : String, path_info : PathInfo?) : {String?, String?}?
     if (parts = str.split("@npm:")).size > 1
       # <alias>@npm:<name>

packages/commands/install/protocol/file/file.cr

@@ -1,8 +1,11 @@
+require "log"
 require "../../resolver"
 require "../base"
 require "./resolver"
 
 struct Commands::Install::Protocol::File < Commands::Install::Protocol::Base
+  Log = ::Log.for("zap.commands.install.protocol.file")
+
   def self.normalize?(str : String, path_info : PathInfo?) : {String?, String?}?
     return nil unless path_info
     path_str = path_info.path.to_s

packages/commands/install/protocol/git/git.cr

@@ -1,9 +1,12 @@
+require "log"
 require "../base"
 require "./resolver"
 require "shared/constants"
 require "concurrency/dedupe_lock"
 
 struct Commands::Install::Protocol::Git < Commands::Install::Protocol::Base
+  Log = ::Log.for("zap.commands.install.protocol.git")
+
   Concurrency::DedupeLock::Global.setup(:clone, Data::Package)
 
   def self.normalize?(str : String, path_info : PathInfo?) : {String?, String?}?

packages/commands/install/protocol/registry/registry.cr

@@ -1,7 +1,10 @@
+require "log"
 require "../base"
 require "./resolver"
 
 struct Commands::Install::Protocol::Registry < Commands::Install::Protocol::Base
+  Log = ::Log.for("zap.commands.install.protocol.registry")
+
   # [<@scope>/]<name>
   # [<@scope>/]<name>@<tag>
   # [<@scope>/]<name>@<version range>

packages/commands/install/protocol/registry/resolver.cr

@@ -1,3 +1,4 @@
+require "log"
 require "fetch"
 require "shared/constants"
 require "../base"
@@ -9,6 +10,8 @@ struct Commands::Install::Protocol::Registry < Commands::Install::Protocol::Base
 end
 
 struct Commands::Install::Protocol::Registry::Resolver < Commands::Install::Protocol::Resolver
+  Log = ::Log.for("zap.commands.install.protocol.registry.resolver")
+
   @clients : RegistryClients
   @client_pool : Fetch(Manifest)
   @package_name : String
@@ -116,7 +119,7 @@ struct Commands::Install::Protocol::Registry::Resolver < Commands::Install::Prot
             end
           rescue e
             state.store.remove_package(metadata)
-            raise e
+            raise Exception.new("Unable to download package #{metadata.name}@#{metadata.version} from #{tarball_url}: #{e.message}", e)
           end
           true
         end

packages/commands/install/protocol/resolver.cr

@@ -7,6 +7,8 @@ require "./aliased"
 require "../state"
 
 abstract struct Commands::Install::Protocol::Resolver
+  Log = ::Log.for("zap.commands.install.protocol.resolver")
+
   alias Specifier = String | Semver::Range
 
   getter name : (String | Aliased)?

packages/commands/install/protocol/tarball_url/tarball_url.cr

@@ -1,7 +1,10 @@
+require "log"
 require "../base"
 require "./resolver"
 
 struct Commands::Install::Protocol::TarballUrl < Commands::Install::Protocol::Base
+  Log = ::Log.for("zap.commands.install.protocol.tarball_url")
+
   def self.normalize?(str : String, path_info : PathInfo?) : {String?, String?}?
     # <tarball url>
     if str.starts_with?("https://") || str.starts_with?("http://")

packages/commands/install/protocol/workspace/workspace.cr

@@ -1,7 +1,10 @@
+require "log"
 require "../base"
 require "./resolver"
 
 struct Commands::Install::Protocol::Workspace < Commands::Install::Protocol::Base
+  Log = ::Log.for("zap.commands.install.protocol.workspace")
+
   def self.normalize?(str : String, path_info : PathInfo?) : {String?, String?}?
     nil
   end

packages/commands/install/registry_clients.cr

@@ -6,9 +6,9 @@ require "fetch"
 # The pools are lazily initialized and cached.
 class Commands::Install::RegistryClients
   # The pool of clients for each registry
-  @client_pool_by_registry : Hash(String, Fetch(Manifest)) = Hash(String, Fetch(Manifest)).new
+  @@client_pool_by_registry : Hash(String, Fetch(Manifest)) = Hash(String, Fetch(Manifest)).new
   # Lock to synchronize access to the pools
-  @client_pool_by_registry_lock = Mutex.new
+  @@client_pool_by_registry_lock = Mutex.new
 
   # Initialize a new registry clients pool with the following arguments:
   # - store_path: path to the store where the metadata will be cached
@@ -26,27 +26,29 @@ class Commands::Install::RegistryClients
 
   # Returns the client pool for the given registry url or creates a new one if it doesn't exist.
   def get_or_init_pool(url : String) : Fetch(Manifest)
-    @client_pool_by_registry_lock.synchronize do
-      @client_pool_by_registry[url] ||= init_client_pool(url)
+    @@client_pool_by_registry_lock.synchronize do
+      @@client_pool_by_registry[url] ||= init_client_pool(url)
     end
   end
 
   # Attempts to find a matching client pool for the given (tarball) url or creates a new one if it doesn't exist.
   # Strips the path from the url before matching.
   def find_or_init_pool(url : String) : {String, Fetch(Manifest)}
-    # Find if an existing pool matches the url
-    @client_pool_by_registry.find do |registry_url, _|
-      url.starts_with?(registry_url)
-    end || begin
-      # Otherwise create a new pool for the url hostname
-      uri = URI.parse(url)
-      # Remove the path - because it is impossible to infer based on the tarball url
-      uri.path = "/"
-      uri_str = uri.to_s
-      pool = init_client_pool(uri.to_s).tap do |pool|
-        @client_pool_by_registry[pool.base_url] = pool
+    @@client_pool_by_registry_lock.synchronize do
+      # Find if an existing pool matches the url
+      @@client_pool_by_registry.find do |registry_url, _|
+        url.starts_with?(registry_url)
+      end || begin
+        # Otherwise create a new pool for the url hostname
+        uri = URI.parse(url)
+        # Remove the path - because it is impossible to infer based on the tarball url
+        uri.path = "/"
+        uri_str = uri.to_s
+        pool = init_client_pool(uri.to_s).tap do |pool|
+          @@client_pool_by_registry[pool.base_url] = pool
+        end
+        {uri_str, pool}
       end
-      {uri_str, pool}
     end
   end
 

packages/commands/install/resolver.cr

@@ -11,7 +11,7 @@ require "./protocol/resolver"
 require "./protocol"
 
 module Commands::Install::Resolver
-  Log = ::Log.for("zap.resolver")
+  Log = ::Log.for("zap.commands.install.resolver")
 
   alias Pipeline = ::Concurrency::Pipeline
 

packages/commands/why/why.cr

@@ -1,4 +1,9 @@
+require "log"
+require "data/lockfile"
+
 module Commands::Why
+  Log = ::Log.for("zap.commands.why")
+
   alias PackageResult = {root: Data::Lockfile::Root, ancestors: Deque({Data::Package, DependencyType}), type: DependencyType}
 
   DEPENDS_ON_CHAR               = '←'

packages/data/package/fields/utility.cr

@@ -100,7 +100,7 @@ class Data::Package
 
     # A more path-friendly key.
     internal { getter hashed_key : String do
-      "#{name}@#{version}__#{dist.class.to_s.split("::").last.downcase}:#{Digest::SHA1.hexdigest(key)}"
+      "#{name}@#{version}__#{dist.class.to_s.split("::").last.downcase}:#{Digest::SHA1.hexdigest(key)}".gsub(':', '_')
     end }
 
     internal { safe_property transitive_overrides : Concurrency::SafeSet(Overrides::Override)? = nil }

packages/data/package/scripts/scripts.cr

@@ -146,14 +146,14 @@ class Data::Package
       pipeline.await
     end
 
+    # See: https://docs.npmjs.com/cli/commands/npm-run-script
     def self.run_script(command : String, chdir : Path | String, config : Core::Config, raise_on_error_code = true, output_io = nil, stdin = Process::Redirect::Close, **args, &block : String, Symbol ->)
       return if command.empty?
-      Log.debug {
-        "Running script: #{command} #{Utils::Macros.args_str}"
-      }
       output = (config.silent ? nil : output_io) || IO::Memory.new
-      # See: https://docs.npmjs.com/cli/commands/npm-run-script
       env = make_env(chdir, config)
+      Log.debug {
+        "Running script: #{command}\n#{Utils::Macros.args_str}\nenvironment: #{env}"
+      }
       yield command, :before
       status = Process.run(command, **args, shell: true, env: env, chdir: chdir.to_s, output: output, input: stdin, error: output)
       if !status.success? && raise_on_error_code

packages/fetch/cache.cr

@@ -4,6 +4,8 @@ require "utils/misc"
 
 class Fetch(T)
   abstract class Cache(T)
+    Log = ::Log.for("zap.fetch.cache")
+
     abstract def get(key_str : String, etag : String?) : T?
     abstract def get(key_str : String, &etag : -> String?) : T?
     abstract def set(key_str : String, value : T, expiry : Time::Span?, etag : String?) : Nil

packages/fetch/fetch.cr

@@ -23,6 +23,7 @@ class Fetch(T)
     &block : HTTP::Client ->
   )
     @pool = Concurrency::Pool(HTTP::Client).new(@pool_max_size) do
+      Log.debug { "Creating new client for #{@base_url}" }
       HTTP::Client.new(URI.parse(base_url)).tap do |client|
         block.call(client)
       end
@@ -41,7 +42,7 @@ class Fetch(T)
         begin
           break yield client
         rescue e
-          ::Log.debug { e.message.colorize.red.to_s + Shared::Constants::NEW_LINE + e.backtrace.map { |line| "\t#{line}" }.join(Shared::Constants::NEW_LINE).colorize.red.to_s }
+          Log.debug { e.message.colorize.red.to_s + Shared::Constants::NEW_LINE + e.backtrace.map { |line| "\t#{line}" }.join(Shared::Constants::NEW_LINE).colorize.red.to_s }
           client.close
           sleep 0.5.seconds * retry_count
           raise e if retry_count >= retry_attempts

packages/git/remote.cr

@@ -1,6 +1,8 @@
 require "log"
 
 class Git::Remote
+  Log = ::Log.for("zap.git.remote")
+
   # See: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#git-urls-as-dependencies
   # <protocol>://[<user>[:<password>]@]<hostname>[:<port>][:][/]<path>[#<commit-ish> | #semver:<semver>]
   GIT_URL_REGEX = /(?:git\+)?(?<protocol>git|ssh|http|https|file):\/\/(?:(?<user>[^:@]+)?(:(?<password>[^@]+))?@)?(?<hostname>[^:\/]+)(:(?<port>\d+))?[\/:](?<path>[^#]+)((?:#semver:(?<semver>[^:]+))|(?:#(?<commitish>[^:]+)))?/
@@ -118,7 +120,7 @@ class Git::Remote
   def self.run(command : String, stdio : Process::Stdio? = nil, **extra) : Nil
     command_and_args = command.split(/\s+/)
     output = stdio || Process::Redirect::Inherit
-    ::Log.debug { "Spawning: #{command_and_args} (#{extra})" }
+    Log.debug { "Spawning: #{command_and_args} (#{extra})" }
     status = Process.run(command_and_args[0], **extra, args: command_and_args[1..]? || nil, output: output, error: output)
     unless status.success?
       Fiber.yield
@@ -130,7 +132,7 @@ class Git::Remote
     command_and_args = command.split(/\s+/)
     stderr = IO::Memory.new
     stdout = IO::Memory.new
-    ::Log.debug { "Spawning: #{command_and_args} (#{extra})" }
+    Log.debug { "Spawning: #{command_and_args} (#{extra})" }
     status = Process.run(command_and_args[0], **extra, args: command_and_args[1..]? || nil, output: stdout, error: stderr)
     unless status.success?
       raise stderr.to_s