From d465298b80038e456111c2090025f5cfbdb50bf2 Mon Sep 17 00:00:00 2001
From: Viet Hoang <1300077+vietqhoang@users.noreply.github.com>
Date: Thu, 24 Oct 2024 12:44:30 +0900
Subject: [PATCH 1/8] =?UTF-8?q?Update=20packager=E2=80=99s=20`package=3F`?=
 =?UTF-8?q?=20to=20return=20Boolean=20type?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The prior implementation returned a `MatchData` object if there is a match or `nil` if there is none. Although the existence of the object is truthy, given the method is interrogative my expectation is it should explicitly return a Boolean.

This is being changed beause I want to use the `importmap.match` to extract the line from the file and pull the pin options. This is set up for keeping things DRY and to keep the commits simple for ease of following along the changes.
---
 lib/importmap/packager.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/importmap/packager.rb b/lib/importmap/packager.rb
index 76f0661..e55f6e6 100644
--- a/lib/importmap/packager.rb
+++ b/lib/importmap/packager.rb
@@ -48,7 +48,7 @@ def vendored_pin_for(package, url)
   end
 
   def packaged?(package)
-    importmap.match(/^pin ["']#{package}["'].*$/)
+    importmap.match(/^pin ["']#{package}["'].*$/).present?
   end
 
   def download(package, url)

From 41de67aa46792cf13be970be535785e52b81cb85 Mon Sep 17 00:00:00 2001
From: Viet Hoang <1300077+vietqhoang@users.noreply.github.com>
Date: Thu, 24 Oct 2024 14:29:21 +0900
Subject: [PATCH 2/8] Add `pin_options_for_package`

This returns any options present for the pinned package.

Variations of lines tested against the `raw_options` regex can be viewed here: https://rubular.com/r/RlOaaKZsbAXfEz

Variation of options tested against the option regex can be viewed here: https://rubular.com/r/qPUodCavWU46GX

The method will be used to repopulate the options when the pinned package is updated.

For the test case I changed the set up to use a fixture. I found editting the dummy apps importmap file to break other tests. Instead of spending time reconciling those tests I went with creating a fixture to be used for the particular test file.
---
 lib/importmap/packager.rb                     | 32 ++++++++++++++++++-
 .../pins_with_various_options_importmap.rb    |  6 ++++
 test/packager_test.rb                         |  9 +++++-
 3 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100644 test/fixtures/files/pins_with_various_options_importmap.rb

diff --git a/lib/importmap/packager.rb b/lib/importmap/packager.rb
index e55f6e6..a2f65cd 100644
--- a/lib/importmap/packager.rb
+++ b/lib/importmap/packager.rb
@@ -48,7 +48,7 @@ def vendored_pin_for(package, url)
   end
 
   def packaged?(package)
-    importmap.match(/^pin ["']#{package}["'].*$/).present?
+    package_line_in_importmap(package).present?
   end
 
   def download(package, url)
@@ -62,6 +62,21 @@ def remove(package)
     remove_package_from_importmap(package)
   end
 
+  def pin_options_for_package(package)
+    line = package_line_in_importmap(package) || ""
+    raw_options = line.match(/^#{base_package_line_regex(package)}?,[\s+]?(?<pin_options>.*) #.*$/)
+
+    return {} if raw_options.blank?
+
+    normalized_delimiter_raw_options = raw_options[:pin_options].gsub(/,\s/, ',').split(',')
+
+    normalized_delimiter_raw_options.each_with_object({}) do |option, hash|
+      match_data = option.match(/^(?<option_name>[^:]*):[\s+]?["']?(?<option_value>.*[^"'])["']?$/)
+
+      hash[match_data[:option_name]] = cast_option_value(match_data[:option_value])
+    end
+  end
+
   private
     def post_json(body)
       Net::HTTP.post(self.class.endpoint, body.to_json, "Content-Type" => "application/json")
@@ -146,4 +161,19 @@ def package_filename(package)
     def extract_package_version_from(url)
       url.match(/@\d+\.\d+\.\d+/)&.to_a&.first
     end
+
+    def package_line_in_importmap(package)
+      importmap.match(/^#{base_package_line_regex(package)}.*$/).try(:[], 0)
+    end
+
+    def base_package_line_regex(package)
+      /pin ["']#{package}["']/
+    end
+
+    def cast_option_value(object)
+      return true if object == "true"
+      return false if object == "false"
+
+      object
+    end
 end
diff --git a/test/fixtures/files/pins_with_various_options_importmap.rb b/test/fixtures/files/pins_with_various_options_importmap.rb
new file mode 100644
index 0000000..5c41266
--- /dev/null
+++ b/test/fixtures/files/pins_with_various_options_importmap.rb
@@ -0,0 +1,6 @@
+pin_all_from "app/assets/javascripts"
+
+pin "md5", to: "https://cdn.skypack.dev/md5", preload: true # 1.0.2
+pin "not_there", to: "nowhere.js", preload: false # 1.9.1
+pin "some_file" # 0.2.1
+pin "another_file",to:'another_file.js' # @0.0.16
diff --git a/test/packager_test.rb b/test/packager_test.rb
index 29ce7f6..e95f6f2 100644
--- a/test/packager_test.rb
+++ b/test/packager_test.rb
@@ -3,7 +3,7 @@
 require "minitest/mock"
 
 class Importmap::PackagerTest < ActiveSupport::TestCase
-  setup { @packager = Importmap::Packager.new(Rails.root.join("config/importmap.rb")) }
+  setup { @packager = Importmap::Packager.new(Rails.root.join("../fixtures/files/pins_with_various_options_importmap.rb")) }
 
   test "successful import with mock" do
     response = Class.new do
@@ -55,4 +55,11 @@ def code() "200" end
     assert_equal %(pin "react" # @17.0.2), @packager.vendored_pin_for("react", "https://cdn/react@17.0.2")
     assert_equal %(pin "javascript/react", to: "javascript--react.js" # @17.0.2), @packager.vendored_pin_for("javascript/react", "https://cdn/react@17.0.2")
   end
+
+  test "pin_options_for_package" do
+    assert_equal ({ "to" => "https://cdn.skypack.dev/md5", "preload" => true }), @packager.pin_options_for_package('md5')
+    assert_equal ({ "to" => "nowhere.js", "preload" => false }), @packager.pin_options_for_package('not_there')
+    assert_equal ({ }), @packager.pin_options_for_package('some_file')
+    assert_equal ({ "to" => "another_file.js" }), @packager.pin_options_for_package('another_file')
+  end
 end

From 3eaf7a16ec80cd20e06bef83499bf8b7228afee6 Mon Sep 17 00:00:00 2001
From: Viet Hoang <1300077+vietqhoang@users.noreply.github.com>
Date: Thu, 24 Oct 2024 15:17:22 +0900
Subject: [PATCH 3/8] Update `packager#vendored_pin_for`

The method now preserves the pin option if it is present.
---
 lib/importmap/packager.rb | 14 ++++++++------
 test/packager_test.rb     |  1 +
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/importmap/packager.rb b/lib/importmap/packager.rb
index a2f65cd..118adf9 100644
--- a/lib/importmap/packager.rb
+++ b/lib/importmap/packager.rb
@@ -39,12 +39,14 @@ def pin_for(package, url)
   def vendored_pin_for(package, url)
     filename = package_filename(package)
     version  = extract_package_version_from(url)
-
-    if "#{package}.js" == filename
-      %(pin "#{package}" # #{version})
-    else
-      %(pin "#{package}", to: "#{filename}" # #{version})
-    end
+    line_formatted_pin_options = pin_options_for_package(package).except("to").map { |option, value| %(#{option}: #{value.is_a?(String) ? %("#{value}") : value}) }
+    pin_components = [
+      %(pin "#{package}"),
+      (%(to: "#{filename}") unless "#{package}.js" == filename),
+      *line_formatted_pin_options
+    ].compact
+
+    %(#{pin_components.join(", ")} # #{version})
   end
 
   def packaged?(package)
diff --git a/test/packager_test.rb b/test/packager_test.rb
index e95f6f2..ee3e0c2 100644
--- a/test/packager_test.rb
+++ b/test/packager_test.rb
@@ -54,6 +54,7 @@ def code() "200" end
   test "vendored_pin_for" do
     assert_equal %(pin "react" # @17.0.2), @packager.vendored_pin_for("react", "https://cdn/react@17.0.2")
     assert_equal %(pin "javascript/react", to: "javascript--react.js" # @17.0.2), @packager.vendored_pin_for("javascript/react", "https://cdn/react@17.0.2")
+    assert_equal %(pin "md5", preload: true # @2.1.3), @packager.vendored_pin_for("md5", "https://cdn/md5@2.1.3")
   end
 
   test "pin_options_for_package" do

From e36fe930b831ae0ec916140d996878ce269d8a4a Mon Sep 17 00:00:00 2001
From: Viet Hoang <1300077+vietqhoang@users.noreply.github.com>
Date: Thu, 24 Oct 2024 15:30:42 +0900
Subject: [PATCH 4/8] Simplify normalization of the delimiter

I found the `split` can take in a regular expression.
---
 lib/importmap/packager.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/importmap/packager.rb b/lib/importmap/packager.rb
index 118adf9..726ae25 100644
--- a/lib/importmap/packager.rb
+++ b/lib/importmap/packager.rb
@@ -70,9 +70,7 @@ def pin_options_for_package(package)
 
     return {} if raw_options.blank?
 
-    normalized_delimiter_raw_options = raw_options[:pin_options].gsub(/,\s/, ',').split(',')
-
-    normalized_delimiter_raw_options.each_with_object({}) do |option, hash|
+    raw_options[:pin_options].split(/,\s|,/).each_with_object({}) do |option, hash|
       match_data = option.match(/^(?<option_name>[^:]*):[\s+]?["']?(?<option_value>.*[^"'])["']?$/)
 
       hash[match_data[:option_name]] = cast_option_value(match_data[:option_value])

From 9709923ba132161fe38b141d6f7b36df25932eb9 Mon Sep 17 00:00:00 2001
From: Viet Hoang <1300077+vietqhoang@users.noreply.github.com>
Date: Thu, 24 Oct 2024 15:33:05 +0900
Subject: [PATCH 5/8] Just use the optional character option

---
 lib/importmap/packager.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/importmap/packager.rb b/lib/importmap/packager.rb
index 726ae25..237b9b6 100644
--- a/lib/importmap/packager.rb
+++ b/lib/importmap/packager.rb
@@ -70,7 +70,7 @@ def pin_options_for_package(package)
 
     return {} if raw_options.blank?
 
-    raw_options[:pin_options].split(/,\s|,/).each_with_object({}) do |option, hash|
+    raw_options[:pin_options].split(/,\s?/).each_with_object({}) do |option, hash|
       match_data = option.match(/^(?<option_name>[^:]*):[\s+]?["']?(?<option_value>.*[^"'])["']?$/)
 
       hash[match_data[:option_name]] = cast_option_value(match_data[:option_value])

From c110d4a1a93bf85dc402c825e9266d876549854f Mon Sep 17 00:00:00 2001
From: Viet Hoang <1300077+vietqhoang@users.noreply.github.com>
Date: Thu, 24 Oct 2024 15:54:08 +0900
Subject: [PATCH 6/8] Add test to make it clear any defined option is carried
 over

---
 test/fixtures/files/pins_with_various_options_importmap.rb | 1 +
 test/packager_test.rb                                      | 1 +
 2 files changed, 2 insertions(+)

diff --git a/test/fixtures/files/pins_with_various_options_importmap.rb b/test/fixtures/files/pins_with_various_options_importmap.rb
index 5c41266..3e0704f 100644
--- a/test/fixtures/files/pins_with_various_options_importmap.rb
+++ b/test/fixtures/files/pins_with_various_options_importmap.rb
@@ -4,3 +4,4 @@
 pin "not_there", to: "nowhere.js", preload: false # 1.9.1
 pin "some_file" # 0.2.1
 pin "another_file",to:'another_file.js' # @0.0.16
+pin "random", random_option: "foobar" # 7.7.7
diff --git a/test/packager_test.rb b/test/packager_test.rb
index ee3e0c2..b4dc12d 100644
--- a/test/packager_test.rb
+++ b/test/packager_test.rb
@@ -55,6 +55,7 @@ def code() "200" end
     assert_equal %(pin "react" # @17.0.2), @packager.vendored_pin_for("react", "https://cdn/react@17.0.2")
     assert_equal %(pin "javascript/react", to: "javascript--react.js" # @17.0.2), @packager.vendored_pin_for("javascript/react", "https://cdn/react@17.0.2")
     assert_equal %(pin "md5", preload: true # @2.1.3), @packager.vendored_pin_for("md5", "https://cdn/md5@2.1.3")
+    assert_equal %(pin "random", random_option: "foobar" # @8.8.8), @packager.vendored_pin_for("random", "https://cdn/random@8.8.8")
   end
 
   test "pin_options_for_package" do

From 9e357503a78ec7fa3bf8231f6806c14e19e933d6 Mon Sep 17 00:00:00 2001
From: Viet Hoang <1300077+vietqhoang@users.noreply.github.com>
Date: Thu, 24 Oct 2024 16:31:37 +0900
Subject: [PATCH 7/8] Update assert to show multiple options are carried over

---
 test/fixtures/files/pins_with_various_options_importmap.rb | 2 +-
 test/packager_test.rb                                      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/fixtures/files/pins_with_various_options_importmap.rb b/test/fixtures/files/pins_with_various_options_importmap.rb
index 3e0704f..e4a9bbf 100644
--- a/test/fixtures/files/pins_with_various_options_importmap.rb
+++ b/test/fixtures/files/pins_with_various_options_importmap.rb
@@ -4,4 +4,4 @@
 pin "not_there", to: "nowhere.js", preload: false # 1.9.1
 pin "some_file" # 0.2.1
 pin "another_file",to:'another_file.js' # @0.0.16
-pin "random", random_option: "foobar" # 7.7.7
+pin "random", random_option: "foobar", hello: "world" # 7.7.7
diff --git a/test/packager_test.rb b/test/packager_test.rb
index b4dc12d..1d173e0 100644
--- a/test/packager_test.rb
+++ b/test/packager_test.rb
@@ -55,7 +55,7 @@ def code() "200" end
     assert_equal %(pin "react" # @17.0.2), @packager.vendored_pin_for("react", "https://cdn/react@17.0.2")
     assert_equal %(pin "javascript/react", to: "javascript--react.js" # @17.0.2), @packager.vendored_pin_for("javascript/react", "https://cdn/react@17.0.2")
     assert_equal %(pin "md5", preload: true # @2.1.3), @packager.vendored_pin_for("md5", "https://cdn/md5@2.1.3")
-    assert_equal %(pin "random", random_option: "foobar" # @8.8.8), @packager.vendored_pin_for("random", "https://cdn/random@8.8.8")
+    assert_equal %(pin "random", random_option: "foobar", hello: "world" # @8.8.8), @packager.vendored_pin_for("random", "https://cdn/random@8.8.8")
   end
 
   test "pin_options_for_package" do

From 1fca7d78b14cfa409bf3e6eabbba97675aade929 Mon Sep 17 00:00:00 2001
From: Viet Hoang <1300077+vietqhoang@users.noreply.github.com>
Date: Fri, 25 Oct 2024 07:34:54 +0900
Subject: [PATCH 8/8] Preserve order of `to` pin option

See comment for context: https://github.com/rails/importmap-rails/pull/274#discussion_r1815765685
---
 lib/importmap/packager.rb                             | 11 +++++------
 .../files/pins_with_various_options_importmap.rb      |  1 +
 test/packager_test.rb                                 |  1 +
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/importmap/packager.rb b/lib/importmap/packager.rb
index 237b9b6..32e642e 100644
--- a/lib/importmap/packager.rb
+++ b/lib/importmap/packager.rb
@@ -39,12 +39,11 @@ def pin_for(package, url)
   def vendored_pin_for(package, url)
     filename = package_filename(package)
     version  = extract_package_version_from(url)
-    line_formatted_pin_options = pin_options_for_package(package).except("to").map { |option, value| %(#{option}: #{value.is_a?(String) ? %("#{value}") : value}) }
-    pin_components = [
-      %(pin "#{package}"),
-      (%(to: "#{filename}") unless "#{package}.js" == filename),
-      *line_formatted_pin_options
-    ].compact
+    line_formatted_pin_options =
+      pin_options_for_package(package).
+        tap { |options| "#{package}.js" == filename ? options.delete("to") : options["to"] = filename }.
+        map { |option, value| %(#{option}: #{value.is_a?(String) ? %("#{value}") : value}) }
+    pin_components = [%(pin "#{package}"), *line_formatted_pin_options]
 
     %(#{pin_components.join(", ")} # #{version})
   end
diff --git a/test/fixtures/files/pins_with_various_options_importmap.rb b/test/fixtures/files/pins_with_various_options_importmap.rb
index e4a9bbf..1285857 100644
--- a/test/fixtures/files/pins_with_various_options_importmap.rb
+++ b/test/fixtures/files/pins_with_various_options_importmap.rb
@@ -5,3 +5,4 @@
 pin "some_file" # 0.2.1
 pin "another_file",to:'another_file.js' # @0.0.16
 pin "random", random_option: "foobar", hello: "world" # 7.7.7
+pin "javascript/typescript", preload: true, to: "https://cdn.skypack.dev/typescript" # 0.0.0
diff --git a/test/packager_test.rb b/test/packager_test.rb
index 1d173e0..baced4d 100644
--- a/test/packager_test.rb
+++ b/test/packager_test.rb
@@ -56,6 +56,7 @@ def code() "200" end
     assert_equal %(pin "javascript/react", to: "javascript--react.js" # @17.0.2), @packager.vendored_pin_for("javascript/react", "https://cdn/react@17.0.2")
     assert_equal %(pin "md5", preload: true # @2.1.3), @packager.vendored_pin_for("md5", "https://cdn/md5@2.1.3")
     assert_equal %(pin "random", random_option: "foobar", hello: "world" # @8.8.8), @packager.vendored_pin_for("random", "https://cdn/random@8.8.8")
+    assert_equal %(pin "javascript/typescript", preload: true, to: "javascript--typescript.js" # @0.0.1), @packager.vendored_pin_for("javascript/typescript", "https://cdn/typescript@0.0.1")
   end
 
   test "pin_options_for_package" do