fix(cli): read an array preload back whatever quotes it is written in

CHANGELOG.md

@@ -284,6 +284,17 @@
   for it, whether that pin is vendored, recording the CDN in its comment, or
   remote, carrying it in the URL with no comment at all.
 
+- **An array `preload:` survives a rewrite however it is quoted, and
+  `preload: []` stays `preload: []`.** The option was read back through
+  `JSON.parse`, so `pin 'md5', preload: ['admin']` — single quotes being a
+  supported pin shape everywhere else in this gem — took down every command
+  that reads a pin with `JSON::ParserError: unexpected character`. And
+  `preload: []` matched nothing at all, so `pin`, `update` and `pristine`
+  dropped it and quietly restored the `preload: true` default on a package the
+  app had asked to preload for no entry point. `config/importmap.rb` is Ruby,
+  not JSON: the entry points are scanned out of the literal now, and an empty
+  array is written back as one.
+
 ## 1.1.0
 
 ### Added

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

@@ -204,8 +204,10 @@ def options_survive
     DocsUI::Section("Pin options survive a rewrite") do
       md <<~'MD'
         When a pin is rewritten — by `pin`, `update` or `pristine` — the options on
-        it are carried over: `preload: false`, `preload: "admin"`, `integrity: true`
-        and `integrity: false` all stay. An explicit `integrity:` *hash* is dropped
+        it are carried over: `preload: false`, `preload: "admin"`,
+        `preload: ["admin", "app"]`, `preload: []`, `integrity: true` and
+        `integrity: false` all stay, whether the pin is written with double quotes
+        or single ones. An explicit `integrity:` *hash* is dropped
         when the URL changes, since the old hash would no longer match the new file;
         see [Subresource integrity](/docs/integrity) for pinning fresh hashes.
 

lib/importmap/packager.rb

@@ -12,7 +12,10 @@ class Importmap::Packager
   include Importmap::HttpRetries
 
   PIN_REGEX = /#{Importmap::Map::PIN_REGEX}(.*)/.freeze # :nodoc:
-  PRELOAD_OPTION_REGEXP = /preload:\s*(\[[^\]]+\]|true|false|["'][^"']*["'])/.freeze # :nodoc:
+  # The bracketed form matches an empty array too: `preload: []` is a pin an
+  # app wrote, and a rewrite that dropped it would start preloading the package
+  # on every page.
+  PRELOAD_OPTION_REGEXP = /preload:\s*(\[[^\]]*\]|true|false|["'][^"']*["'])/.freeze # :nodoc:
   TO_OPTION_REGEXP = /to:\s*["']([^"']*)["']/.freeze # :nodoc:
   # Only the booleans: a hash string is tied to the file it was computed for,
   # so a rewrite that changes the URL has to drop it.
@@ -544,16 +547,24 @@ def preload_from_string(value)
       when "false"
         false
       when /^\[.*\]$/
-        JSON.parse(value)
+        # config/importmap.rb is Ruby, not JSON, and a single-quoted pin is a
+        # supported shape here, so the entry points are scanned out of the
+        # literal rather than parsed. JSON.parse raised on every one of them.
+        value.scan(/["']([^"']*)["']/).flatten
       else
         value.gsub(/["']/, "")
       end
     end
 
     def preload(preloads)
+      # nil is "the pin carries no preload:"; [] is the pin carrying one that
+      # names no entry point. Array() flattens both to [], so the difference
+      # has to be read before it.
+      return "" if preloads.nil?
+
       case Array(preloads)
       in []
-        ""
+        %(, preload: [])
       in ["true"] | [true]
         %(, preload: true)
       in ["false"] | [false]

test/fixtures/files/single_quote_array_preload_import_map.rb

@@ -0,0 +1,3 @@
+pin 'md5', preload: ['admin'] # @2.2.0
+pin 'charenc', preload: ['admin', 'app'] # @0.0.2
+pin 'crypt', preload: [] # @0.0.2

test/packager_single_quotes_test.rb

@@ -19,4 +19,18 @@ class Importmap::PackagerSingleQuotesTest < ActiveSupport::TestCase
     assert @packager.remove("md5")
     assert_not @packager.packaged?("md5")
   end
+
+  test "extract_existing_pin_options with single quotes" do
+    assert_equal({ preload: true, to: "https://cdn.skypack.dev/md5", integrity: false },
+                 @packager.extract_existing_pin_options("md5")["md5"])
+  end
+
+  test "an array preload with single quotes is read and written back without its quotes mattering" do
+    packager = Importmap::Packager.new(file_fixture("single_quote_array_preload_import_map.rb"))
+
+    assert_equal [ "admin", "app" ], packager.extract_existing_pin_options("charenc")["charenc"][:preload]
+    assert_equal %(pin "charenc", preload: ["admin", "app"]),
+                 packager.pin_for("charenc", preloads: [ "admin", "app" ])
+    assert_equal %(pin "crypt", preload: []), packager.pin_for("crypt", preloads: [])
+  end
 end

test/packager_test.rb

@@ -1393,6 +1393,44 @@ def body() %(import"/npm/dep@1.0.0/+esm";export default new Worker(u)) end
     end
   end
 
+  test "extract_existing_pin_options reads an array preload written with single quotes" do
+    packager = Importmap::Packager.new(file_fixture("single_quote_array_preload_import_map.rb"))
+
+    assert_equal({ preload: [ "admin" ] }, extract_options_for_package(packager, "md5"))
+    assert_equal({ preload: [ "admin", "app" ] }, extract_options_for_package(packager, "charenc"))
+    assert_equal({ preload: [] }, extract_options_for_package(packager, "crypt"))
+  end
+
+  test "extract_existing_pin_options keeps an empty array preload" do
+    temp_importmap = create_temp_importmap(<<~PINS)
+      pin "package1", preload: []
+      pin "package2", preload: [], integrity: true
+    PINS
+    packager = Importmap::Packager.new(temp_importmap)
+
+    assert_equal({ preload: [] }, extract_options_for_package(packager, "package1"))
+    assert_equal({ preload: [], integrity: true }, extract_options_for_package(packager, "package2"))
+  end
+
+  test "pin_for writes an empty array preload rather than dropping it" do
+    assert_equal %(pin "react", preload: []), @packager.pin_for("react", preloads: [])
+    assert_equal %(pin "react", preload: [] # @17.0.2),
+                 @packager.vendored_pin_for("react", "https://cdn/react@17.0.2", [])
+    assert_equal %(pin "react"), @packager.pin_for("react", preloads: nil)
+  end
+
+  test "an array preload survives being read and written again" do
+    temp_importmap = create_temp_importmap(<<~PINS)
+      pin 'package1', preload: ['admin', 'app']
+      pin 'package2', preload: []
+    PINS
+    packager = Importmap::Packager.new(temp_importmap)
+
+    assert_equal %(pin "package1", preload: ["admin", "app"]),
+                 packager.pin_for("package1", preloads: extract_options_for_package(packager, "package1")[:preload])
+    assert_equal %(pin "package2", preload: []),
+                 packager.pin_for("package2", preloads: extract_options_for_package(packager, "package2")[:preload])
+  end
   private
     GRAPH_ROOT = "https://ga.jspm.io/npm:pkg@1.0.0/".freeze