fix(cli): a batch a CDN refuses is split before it falls back

CHANGELOG.md

@@ -243,6 +243,39 @@
 
 ### Fixed
 
+- **One package a CDN can't resolve no longer blocks — or silently moves — the
+  rest of the batch.** ([#32](https://github.com/zoolutions/importmap-plus/issues/32))
+  A CDN answers a batch of packages as a whole, so `bin/importmap update` with
+  three outdated packages reported `Couldn't find any packages in
+  ["cheap-ruler", "mapbox-gl", "mermaid"] on jspm`, updated nothing and exited
+  0, because jspm's generator can't build `mermaid`. Commenting out the one
+  package let the other two through. A refused batch of more than one package
+  is now asked for one package at a time, along the path each would have taken
+  alone:
+
+  ```
+  $ bin/importmap pin md5@2.2.0 mermaid@10.6.0
+  jspm couldn't resolve "md5@2.2.0", "mermaid@10.6.0" (No './dist/cytoscape.umd.js' exports subpath defined in cytoscape@3.34.3); asking for each on its own
+  Pinning "md5" to vendor/javascript/md5.js via download from https://ga.jspm.io/npm:md5@2.2.0/md5.js
+  jspm couldn't resolve "mermaid@10.6.0" (No './dist/cytoscape.umd.js' exports subpath defined in cytoscape@3.34.3); trying esm.run
+  Pinning "mermaid" to vendor/javascript/mermaid.js via download from https://cdn.jsdelivr.net/npm/mermaid@10.6.0/+esm
+  ```
+  ```ruby
+  pin "md5" # @2.2.0
+  pin "mermaid" # @10.6.0 (esm.run)
+  ```
+
+  Only the package the CDN refused travels the rest of the chain, so a healthy
+  package is never re-pinned from another CDN — and never has its provenance
+  comment rewritten — because a sibling failed. An explicit `--from`, or a
+  provider a pin records, still asks that one CDN and only that one, now once
+  per package. The batch stays the fast path: it is split only when it is
+  refused and holds more than one package.
+
+  `pin`, `update` and `pristine` now **exit 1** when a package was left
+  unresolved, so `bin/importmap update && git commit` can no longer commit an
+  import map that quietly missed a package. The packages that did resolve are
+  still written.
 - **A CDN that fails mid-crawl leaves the pin alone.** Vendoring a graph makes
   one request per file — 250 of them for `date-fns` — so a 503 that outlives
   the retries is far likelier than it was for a single download. `pin` and

docs/app/views/docs/pages/cli.rb

@@ -111,6 +111,7 @@ def exit_codes
           [ [ :code, "outdated" ], "an unlocked package is outdated" ],
           [ [ :code, "audit" ], "a vulnerability is known for a pinned version" ],
           [ [ :code, "pin / update" ], "a package was skipped — its directory is in the way, or its CDN failed partway through a crawl — while the rest were pinned; the skipped pin is left exactly as it was" ],
+          [ [ :code, "pin / update / pristine" ], "no CDN could resolve a package, its reason printed; the packages that did resolve are still written" ],
           [ [ :code, "update" ], "a named package has no pin, or names are combined with --all; nothing is updated in either case" ],
           [ [ :code, "pristine" ], "a package couldn't be restored the way its pin describes, or a dependency of an esm.run bundle was skipped; the rest are restored" ],
           [ [ :code, "lock / unlock" ], "a named package has no pin, has no version to lock at, or was given with a version" ],

docs/app/views/docs/pages/pinning.rb

@@ -98,8 +98,29 @@ def choosing_a_cdn
         pin comment, so `update` and `pristine` go straight back to it and never
         retry the jspm that couldn't build it. Nothing new is stored anywhere else.
 
-        If no CDN in the chain has the package, each one's reason is printed and the
-        command says so:
+        A CDN answers a batch of packages as a whole, so one spec its generator
+        can't build would be a "no" for every package beside it. When that happens
+        each package is asked for on its own, along the path it would have taken
+        alone — so only the package the CDN actually refused moves to the next one:
+      MD
+      DocsUI::Code(<<~SHELL, lexer: :console)
+        $ ./bin/importmap pin md5@2.2.0 mermaid@10.6.0
+        jspm couldn't resolve "md5@2.2.0", "mermaid@10.6.0" (No './dist/cytoscape.umd.js' exports subpath defined in cytoscape@3.34.3); asking for each on its own
+        Pinning "md5" to vendor/javascript/md5.js via download from https://ga.jspm.io/npm:md5@2.2.0/md5.js
+        jspm couldn't resolve "mermaid@10.6.0" (No './dist/cytoscape.umd.js' exports subpath defined in cytoscape@3.34.3); trying esm.run
+        Pinning "mermaid" to vendor/javascript/mermaid.js via download from https://cdn.jsdelivr.net/npm/mermaid@10.6.0/+esm
+      SHELL
+      DocsUI::Code(<<~RUBY, filename: "config/importmap.rb")
+        pin "md5" # @2.2.0
+        pin "mermaid" # @10.6.0 (esm.run)
+      RUBY
+      md <<~'MD'
+        `md5` keeps the CDN it would have been pinned from alone — a sibling failing
+        is no reason to change a package's provenance.
+
+        If no CDN in the chain has the package, each one's reason is printed, the
+        command says so and it exits 1. The packages that did resolve are still
+        pinned:
       MD
       DocsUI::Code(<<~SHELL, lexer: :console)
         $ ./bin/importmap pin no-such-package
@@ -123,7 +144,9 @@ def choosing_a_cdn
       md <<~'MD'
         `--from` is a choice you made, so it is asked once and never falls back: the
         CDN you named either has the package or reports why it hasn't. The same goes
-        for a package whose pin already records a CDN.
+        for a package whose pin already records a CDN. A batch it refuses is still
+        split — each package is asked of that same CDN on its own, so the ones it
+        has are pinned and only the ones it hasn't are reported.
 
         The CDN is recorded in the pin comment when it isn't jspm, and later commands
         go back to it — an unpkg download stays on unpkg through `update` and

docs/app/views/docs/pages/updating.rb

@@ -46,6 +46,12 @@ def update
         against the CDN. Every other package still updates, and the command exits 1
         so a script knows it didn't do all it was asked.
 
+        A package its CDN can't resolve is reported with the CDN's own reason and
+        left where it is. It doesn't hold back the rest: the CDN is asked for each
+        package on its own once it refuses the batch, so every package it can build
+        still updates, from the CDN its pin names. The command exits 1 to say it
+        didn't do all it was asked.
+
         A package is re-resolved together with the dependencies its CDN lists for it,
         so those move as well, keeping their own pin options. Each package comes back
         from the CDN its pin comment names ([Provenance](/docs/provenance)); a remote
@@ -173,7 +179,8 @@ def pristine
         and skipped — `Couldn't restore "pdfjs-dist": it can't be vendored as a
         single file (workers)` — and the rest of the packages are still restored;
         the command exits non-zero to say it didn't do all of it, as it does when a
-        dependency of an esm.run bundle, pinned on the way, had to be skipped.
+        dependency of an esm.run bundle, pinned on the way, had to be skipped, or
+        when the CDN a pin names couldn't resolve the package at all.
       MD
     end
   end

docs/app/views/docs/pages/upgrading.rb

@@ -51,6 +51,7 @@ def what_changes
           [ "A failed CDN request is a raw backtrace.", "Requests are retried three times with a growing pause; the failure then names the URL." ],
           [ "A download that imports sibling files, spawns a worker or fetches a .wasm is vendored anyway and 404s in the browser.", "It is pinned to its CDN URL instead, the pin says why, and pin --vendor overrides. See below." ],
           [ "pin asks jspm and nothing else; a package its generator can't build reports \"Couldn't find any packages\".", "jspm, then esm.run, then jsDelivr, until one answers. The CDN that did is recorded on the pin. --from turns the fallback off." ],
+          [ "One package a CDN refuses fails the whole command, which still exits 0.", "The refused batch is asked for one package at a time, so the rest are pinned from the CDN they would have come from alone. pin, update and pristine exit 1 when a package was left unresolved." ],
           [ "pin foo takes whatever version jspm has indexed.", "The npm registry decides the version, then every CDN is asked for that one." ],
           [ "A CDN that hands back a UMD bundle is vendored, and the import fails to link in the browser.", "It is pinned to its CDN URL with (remote: not an ES module). pin --vendor overrides." ]
         ]

lib/importmap/batch_resolver.rb

@@ -0,0 +1,133 @@
+require "importmap/provider_chain"
+
+# Resolves a group of specs against a CDN, batch first and then one at a time.
+#
+# The batch is the fast path and stays: one round trip, and jspm resolves the
+# specs' shared dependencies as a single graph, which is what upstream does.
+# But a CDN answers a batch as a whole — one spec its generator can't build is
+# "no" for all of them — so a refused batch of more than one spec says nothing
+# about the specs in it. Each is then asked on its own, along the path it would
+# have taken alone: through the chain when the group named no CDN, of the same
+# provider once when it did. A package that was always going to resolve is
+# pinned from the CDN it was always going to be pinned from, whatever its
+# siblings did.
+#
+# Specs nobody could resolve are collected rather than raised: each pin is
+# independent of the others, so the rest still land and the exit code says the
+# command didn't do all it was asked.
+class Importmap::BatchResolver
+  # The specs no CDN answered for, in the order they were asked.
+  attr_reader :unresolved
+
+  # +on_miss+ is the command's own "couldn't find" reporter, so the sentence a
+  # single spec gets is written in exactly one place.
+  def initialize(packager, on_miss:, chain: Importmap::ProviderChain.new)
+    @packager   = packager
+    @on_miss    = on_miss
+    @chain      = chain
+    @unresolved = []
+  end
+
+  # Yields [ package, url ] for everything that resolved, as it resolves, so a
+  # spec that fails later can't discard the work of one that succeeded earlier.
+  def each_import(specs, env:, from:, fallback:, &block)
+    @named = specs.map { |spec| @packager.package_key_for(spec) }
+    @seen  = {}
+
+    if specs.one?
+      resolve_one(specs.first, env: env, from: from, fallback: fallback, &block)
+    elsif (response = resolve_batch(specs, env: env, from: from, fallback: fallback))
+      emit(specs, response, &block)
+    else
+      specs.each { |spec| resolve_one(spec, env: env, from: from, fallback: fallback, &block) }
+    end
+  end
+
+  private
+    # The batch a group with no CDN of its own sends goes to jspm alone. Handing
+    # the whole list to the next CDN is what moved a healthy package's provenance
+    # when a sibling failed; the specs travel the rest of the chain separately.
+    def resolve_batch(specs, env:, from:, fallback:)
+      provider = fallback ? Importmap::ProviderChain::DEFAULT : from
+      error    = nil
+
+      response =
+        begin
+          @packager.import(*specs, env: env, from: provider)
+        rescue Importmap::Packager::Error => raised
+          error = raised
+          nil
+        end
+
+      return response if response
+
+      report_split(specs, provider, error&.message || @packager.last_import_error, fallback)
+      nil
+    end
+
+    def report_split(specs, provider, reason, fallback)
+      detail = Importmap::ProviderChain.tidy_reason(reason)
+      miss   = if fallback
+        %(#{provider} couldn't resolve #{quoted(specs)})
+      else
+        %(Couldn't find any packages in #{specs.inspect} on #{provider})
+      end
+
+      puts miss + (detail ? %( (#{detail})) : "") + "; asking for each on its own"
+    end
+
+    def resolve_one(spec, env:, from:, fallback:, &block)
+      source = fallback ? Importmap::ProviderChain.to_sentence : from
+
+      if (response = request_one(spec, env: env, from: from, fallback: fallback))
+        emit([ spec ], response, &block)
+      else
+        # A chain that came up empty has already said why each CDN couldn't.
+        @on_miss.call([ spec ], source, reason: fallback ? nil : @packager.last_import_error)
+        @unresolved << spec
+      end
+    rescue Importmap::Packager::Error => error
+      puts %(Couldn't resolve "#{spec}" from #{source}: #{error.message})
+      @unresolved << spec
+    end
+
+    def request_one(spec, env:, from:, fallback:)
+      if fallback
+        @chain.resolve(@packager, [ spec ], env: env) { |_provider, response| response }
+      else
+        @packager.import(spec, env: env, from: from)
+      end
+    end
+
+    # Split responses overlap: each carries the dependencies of its own spec.
+    # A spec the user named answers for itself, so a sibling's response naming
+    # it is a dependency edge and not the answer asked for. Anything else is
+    # taken from the first response that carried it, because rewriting it to a
+    # second URL would move a package the user never mentioned.
+    def emit(specs, response, &block)
+      asked_for = specs.map { |spec| @packager.package_key_for(spec) }
+
+      response[:imports].each do |package, url|
+        next if @named.include?(package) && !asked_for.include?(package)
+
+        if (kept = @seen[package])
+          report_kept(package, kept, url, specs.first) unless kept == url
+        else
+          @seen[package] = url
+          block.call(package, url)
+        end
+      end
+    end
+
+    def report_kept(package, kept, url, spec)
+      puts %(Keeping "#{package}" at #{version(kept)} ("#{spec}" resolved it to #{version(url)}))
+    end
+
+    def version(url)
+      @packager.extract_package_version_from(url) || url
+    end
+
+    def quoted(specs)
+      specs.map { |spec| %("#{spec}") }.join(", ")
+    end
+end

lib/importmap/commands.rb

@@ -4,6 +4,7 @@
 require "importmap/vendored_graph"
 require "importmap/npm"
 require "importmap/provider_chain"
+require "importmap/batch_resolver"
 require "importmap/integrity"
 require "importmap/doctor"
 
@@ -39,7 +40,7 @@ def pin(*packages)
                                 vendor: requested.include?(package) && options[:vendor])
     end
 
-    exit 1 if skipped.any?
+    exit 1 if skipped.any? || resolver.unresolved.any?
   end
 
   desc "lock [*PACKAGES]", "Lock packages at their pinned version"
@@ -84,7 +85,7 @@ def pristine
 
     # skipped: an esm.run bundle's dependency is pinned on the way, and one the
     # CDN failed on is skipped the same way pin skips it.
-    exit 1 if unrestored.any? || skipped.any?
+    exit 1 if unrestored.any? || skipped.any? || resolver.unresolved.any?
   end
 
   desc "json", "Show the full importmap in json"
@@ -166,7 +167,7 @@ def update(*packages)
       end
     end
 
-    exit 1 if unchecked_packages.any? || skipped.any?
+    exit 1 if unchecked_packages.any? || skipped.any? || resolver.unresolved.any?
   end
 
   desc "packages", "Print out packages with version numbers"
@@ -526,28 +527,15 @@ def vendored_minified?(package)
     # another CDN would be a rewrite, not a restore.
     def for_each_import_grouped_by_provider(packages, env:, from: nil, fallback: false, &block)
       packages.group_by { |spec| from || vendored_provider_for(spec) || Importmap::ProviderChain::DEFAULT }.each do |provider, group|
-        if fallback && from.nil? && Importmap::ProviderChain.default?(provider)
-          for_each_import_with_fallback(group, env: env, &block)
-        else
-          for_each_import(group, env: env, from: provider, &block)
-        end
+        resolver.each_import(group, env: env, from: provider,
+          fallback: fallback && from.nil? && Importmap::ProviderChain.default?(provider), &block)
       end
     end
 
-    # A package nobody has pinned yet names no CDN, so it is asked of each in
-    # turn: jspm's generator giving up on a package says something about the
-    # generator, not about the package, and the next CDN often has it. A
-    # provider a pin already records, or one --from names, is a choice somebody
-    # made, and a choice is asked once and reported on.
-    def for_each_import_with_fallback(packages, env:, &block)
-      response = Importmap::ProviderChain.new.resolve(packager, packages, env: env) { |_provider, answer| answer }
-
-      if response
-        response[:imports].each(&block)
-      else
-        # Every CDN has already said why it couldn't, so the summary repeats none of it.
-        handle_package_not_found(packages, Importmap::ProviderChain.to_sentence, reason: nil)
-      end
+    # Shared by every group of a single run, so the exit code can ask it once
+    # whether anything was left unresolved.
+    def resolver
+      @resolver ||= Importmap::BatchResolver.new(packager, on_miss: method(:handle_package_not_found))
     end
 
     # A spec with no version means the latest, and the registry is what knows